Misc refactors 2024-03-14 (#89)

* Adds method to improve cleanup of empty directories

* resolved bug where source metadata worker could call itself in an infinite loop

* Refactored file deletion for media items

* Removed useless filesystem data worker

* Updated task listing fns to take a record directly

* Refactored the way I call workers

* Improved some tests

.credo.exs

@@ -128,7 +128,7 @@
           {Credo.Check.Refactor.MatchInCondition, []},
           {Credo.Check.Refactor.NegatedConditionsInUnless, []},
           {Credo.Check.Refactor.NegatedConditionsWithElse, []},
-          {Credo.Check.Refactor.Nesting, []},
+          {Credo.Check.Refactor.Nesting, [max_nesting: 4]},
           {Credo.Check.Refactor.RedundantWithClauseResult, []},
           {Credo.Check.Refactor.RejectReject, []},
           {Credo.Check.Refactor.UnlessWithElse, []},

lib/pinchflat/boot/pre_job_startup_tasks.ex

@@ -14,6 +14,7 @@ defmodule Pinchflat.Boot.PreJobStartupTasks do
 
   alias Pinchflat.Repo
   alias Pinchflat.Settings
+  alias Pinchflat.Filesystem.FilesystemHelpers
 
   def start_link(opts \\ []) do
     GenServer.start_link(__MODULE__, %{}, opts)
@@ -31,6 +32,7 @@ defmodule Pinchflat.Boot.PreJobStartupTasks do
   @impl true
   def init(state) do
     apply_default_settings()
+    ensure_directories_are_writeable()
     rename_old_job_workers()
 
     {:ok, state}
@@ -41,6 +43,21 @@ defmodule Pinchflat.Boot.PreJobStartupTasks do
     Settings.fetch!(:pro_enabled, false)
   end
 
+  defp ensure_directories_are_writeable do
+    directories = [
+      Application.get_env(:pinchflat, :media_directory),
+      Application.get_env(:pinchflat, :tmpfile_directory),
+      Application.get_env(:pinchflat, :metadata_directory)
+    ]
+
+    Enum.each(directories, fn dir ->
+      file = Path.join([dir, ".keep"])
+
+      # This will fail if the directory is not writeable, stopping boot
+      FilesystemHelpers.write_p!(file, "")
+    end)
+  end
+
   # As part of a large refactor, I ended up moving a bunch of workers around. This
   # is a problem because the workers are stored in the database and the runner
   # will try to run the OLD jobs. This is also why these tasks run before the job
@@ -52,7 +69,6 @@ defmodule Pinchflat.Boot.PreJobStartupTasks do
     rename_map = [
       ["Pinchflat.Workers.MediaIndexingWorker", "Pinchflat.FastIndexing.MediaIndexingWorker"],
       ["Pinchflat.Workers.MediaDownloadWorker", "Pinchflat.Downloading.MediaDownloadWorker"],
-      ["Pinchflat.Workers.FilesystemDataWorker", "Pinchflat.Filesystem.FilesystemDataWorker"],
       ["Pinchflat.Workers.FastIndexingWorker", "Pinchflat.FastIndexing.FastIndexingWorker"],
       ["Pinchflat.Workers.MediaCollectionIndexingWorker", "Pinchflat.SlowIndexing.MediaCollectionIndexingWorker"],
       ["Pinchflat.Workers.DataBackfillWorker", "Pinchflat.Boot.DataBackfillWorker"]

lib/pinchflat/downloading/download_option_builder.ex

@@ -99,7 +99,6 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do
       :"480p" -> [format_sort: "res:480,#{video_codec_options}"]
       :"720p" -> [format_sort: "res:720,#{video_codec_options}"]
       :"1080p" -> [format_sort: "res:1080,#{video_codec_options}"]
-      :"1440p" -> [format_sort: "res:1440,#{video_codec_options}"]
       :"2160p" -> [format_sort: "res:2160,#{video_codec_options}"]
     end
   end

lib/pinchflat/downloading/downloading_helpers.ex

@@ -26,11 +26,7 @@ defmodule Pinchflat.Downloading.DownloadingHelpers do
   def enqueue_pending_download_tasks(%Source{download_media: true} = source) do
     source
     |> Media.list_pending_media_items_for()
-    |> Enum.each(fn media_item ->
+    |> Enum.each(&MediaDownloadWorker.kickoff_with_task/1)
-      %{id: media_item.id}
-      |> MediaDownloadWorker.new()
-      |> Tasks.create_job_with_task(media_item)
-    end)
   end
 
   def enqueue_pending_download_tasks(%Source{download_media: false}) do

lib/pinchflat/downloading/media_download_worker.ex

@@ -6,19 +6,30 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do
     unique: [period: :infinity, states: [:available, :scheduled, :retryable, :executing]],
     tags: ["media_item", "media_fetching"]
 
+  alias __MODULE__
+  alias Pinchflat.Tasks
   alias Pinchflat.Repo
   alias Pinchflat.Media
-  alias Pinchflat.Tasks
   alias Pinchflat.Downloading.MediaDownloader
-  alias Pinchflat.Filesystem.FilesystemDataWorker
 
-  @impl Oban.Worker
+  @doc """
+  Starts the media_item media download worker and creates a task for the media_item.
+
+  Returns {:ok, %Task{}} | {:error, :duplicate_job} | {:error, %Ecto.Changeset{}}
+  """
+  def kickoff_with_task(media_item, opts \\ []) do
+    %{id: media_item.id}
+    |> MediaDownloadWorker.new(opts)
+    |> Tasks.create_job_with_task(media_item)
+  end
+
   @doc """
   For a given media item, download the media alongside any options.
   Does not download media if its source is set to not download media.
 
   Returns :ok | {:ok, %MediaItem{}} | {:error, any, ...any}
   """
+  @impl Oban.Worker
   def perform(%Oban.Job{args: %{"id" => media_item_id}}) do
     media_item =
       media_item_id
@@ -35,22 +46,23 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do
 
   defp download_media_and_schedule_jobs(media_item) do
     case MediaDownloader.download_for_media_item(media_item) do
-      {:ok, _} ->
+      {:ok, updated_media_item} ->
-        schedule_filesystem_data_worker(media_item)
+        compute_and_save_media_filesize(updated_media_item)
-        {:ok, media_item}
+
+        {:ok, updated_media_item}
 
       err ->
         err
     end
   end
 
-  defp schedule_filesystem_data_worker(media_item) do
+  defp compute_and_save_media_filesize(media_item) do
-    %{id: media_item.id}
+    case File.stat(media_item.media_filepath) do
-    |> FilesystemDataWorker.new()
+      {:ok, %{size: size}} ->
-    |> Tasks.create_job_with_task(media_item)
+        Media.update_media_item(media_item, %{media_size_bytes: size})
-    |> case do
+
-      {:ok, task} -> {:ok, task}
+      _ ->
-      {:error, :duplicate_job} -> {:ok, :job_exists}
+        :ok
     end
   end
 end

lib/pinchflat/downloading/media_downloader.ex

@@ -38,6 +38,8 @@ defmodule Pinchflat.Downloading.MediaDownloader do
             media_downloaded_at: DateTime.utc_now(),
             nfo_filepath: determine_nfo_filepath(item_with_preloads, parsed_json),
             metadata: %{
+              # IDEA: might be worth kicking off a job for this since thumbnail fetching
+              # could fail and I want to handle that in isolation
               metadata_filepath: MetadataFileHelpers.compress_and_store_metadata_for(media_item, parsed_json),
               thumbnail_filepath: MetadataFileHelpers.download_and_store_thumbnail_for(media_item, parsed_json)
             }

lib/pinchflat/fast_indexing/fast_indexing_helpers.ex

@@ -6,34 +6,13 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpers do
   """
 
   alias Pinchflat.Media
-  alias Pinchflat.Tasks
   alias Pinchflat.Sources.Source
   alias Pinchflat.FastIndexing.YoutubeRss
-  alias Pinchflat.FastIndexing.FastIndexingWorker
   alias Pinchflat.Downloading.MediaDownloadWorker
   alias Pinchflat.FastIndexing.MediaIndexingWorker
 
   alias Pinchflat.YtDlp.Media, as: YtDlpMedia
 
-  @doc """
-  Starts tasks for running a fast indexing task for a source's media
-  regardless of the source's fast_index state. It's assumed the
-  caller will check for fast_index.
-
-  This is used for running fast index tasks on update. On creation, the
-  fast index is enqueued after the slow index is complete.
-
-  Returns {:ok, %Task{}}.
-  """
-  def kickoff_fast_indexing_task(%Source{} = source) do
-    Tasks.delete_pending_tasks_for(source, "FastIndexingWorker")
-
-    %{id: source.id}
-    # Schedule this one immediately, but future ones will be on an interval
-    |> FastIndexingWorker.new()
-    |> Tasks.create_job_with_task(source)
-  end
-
   @doc """
   Fetches new media IDs from a source's YouTube RSS feed and kicks off indexing tasks
   for any new media items. See comments in `MediaIndexingWorker` for more info on the
@@ -54,9 +33,7 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpers do
     Enum.each(new_media_ids, fn media_id ->
       url = "https://www.youtube.com/watch?v=#{media_id}"
 
-      %{id: source.id, media_url: url}
+      MediaIndexingWorker.kickoff_with_task(source, url)
-      |> MediaIndexingWorker.new()
-      |> Tasks.create_job_with_task(source)
     end)
   end
 
@@ -74,9 +51,7 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpers do
     case maybe_media_item do
       {:ok, media_item} ->
         if source.download_media && Media.pending_download?(media_item) do
-          %{id: media_item.id}
+          MediaDownloadWorker.kickoff_with_task(media_item)
-          |> MediaDownloadWorker.new()
-          |> Tasks.create_job_with_task(media_item)
         end
 
         {:ok, media_item}

lib/pinchflat/fast_indexing/fast_indexing_worker.ex

@@ -12,7 +12,17 @@ defmodule Pinchflat.FastIndexing.FastIndexingWorker do
   alias Pinchflat.Sources.Source
   alias Pinchflat.FastIndexing.FastIndexingHelpers
 
-  @impl Oban.Worker
+  @doc """
+  Starts the source fast indexing worker and creates a task for the source.
+
+  Returns {:ok, %Task{}} | {:error, :duplicate_job} | {:error, %Ecto.Changeset{}}
+  """
+  def kickoff_with_task(source, opts \\ []) do
+    %{id: source.id}
+    |> FastIndexingWorker.new(opts)
+    |> Tasks.create_job_with_task(source)
+  end
+
   @doc """
   Kicks off the fast indexing process for a source, reschedules the job to run again
   once complete. See `MediaCollectionIndexingWorker` and `MediaIndexingWorker` comments
@@ -20,6 +30,7 @@ defmodule Pinchflat.FastIndexing.FastIndexingWorker do
 
   Returns :ok | {:ok, :job_exists} | {:ok, %Task{}}
   """
+  @impl Oban.Worker
   def perform(%Oban.Job{args: %{"id" => source_id}}) do
     source = Sources.get_source!(source_id)
 
@@ -35,10 +46,7 @@ defmodule Pinchflat.FastIndexing.FastIndexingWorker do
   defp reschedule_indexing(source) do
     next_run_in = Source.fast_index_frequency() * 60
 
-    %{id: source.id}
+    case kickoff_with_task(source, schedule_in: next_run_in) do
-    |> FastIndexingWorker.new(schedule_in: next_run_in)
-    |> Tasks.create_job_with_task(source)
-    |> case do
       {:ok, task} -> {:ok, task}
       {:error, :duplicate_job} -> {:ok, :job_exists}
     end

lib/pinchflat/fast_indexing/media_indexing_worker.ex

@@ -8,10 +8,22 @@ defmodule Pinchflat.FastIndexing.MediaIndexingWorker do
 
   require Logger
 
+  alias __MODULE__
+  alias Pinchflat.Tasks
   alias Pinchflat.Sources
   alias Pinchflat.FastIndexing.FastIndexingHelpers
 
-  @impl Oban.Worker
+  @doc """
+  Starts the fast media indexing worker and creates a task for the source.
+
+  Returns {:ok, %Task{}} | {:error, :duplicate_job} | {:error, %Ecto.Changeset{}}
+  """
+  def kickoff_with_task(source, media_url, opts \\ []) do
+    %{id: source.id, media_url: media_url}
+    |> MediaIndexingWorker.new(opts)
+    |> Tasks.create_job_with_task(source)
+  end
+
   @doc """
   Similar to `MediaCollectionIndexingWorker`, but for individual media items.
   Does not reschedule or check anything to do with a source's indexing
@@ -37,6 +49,7 @@ defmodule Pinchflat.FastIndexing.MediaIndexingWorker do
 
   Returns :ok
   """
+  @impl Oban.Worker
   def perform(%Oban.Job{args: %{"id" => source_id, "media_url" => media_url}}) do
     source = Sources.get_source!(source_id)
 

lib/pinchflat/filesystem/filesystem_data_worker.ex

@@ -1,31 +0,0 @@
-defmodule Pinchflat.Filesystem.FilesystemDataWorker do
-  @moduledoc false
-
-  use Oban.Worker,
-    queue: :local_metadata,
-    tags: ["media_item", "media_metadata", "local_metadata"],
-    max_attempts: 1
-
-  alias Pinchflat.Media
-  alias Pinchflat.Filesystem.FilesystemHelpers
-
-  @impl Oban.Worker
-  @doc """
-  For a given media item, compute and save metadata about the file on-disk.
-
-  IDEA: does this have to be a standalone job? I originally split it out
-  so a failure here wouldn't cause a downloader job retry, but I can match
-  for failures so it doesn't retry.
-
-  Returns :ok
-  """
-  def perform(%Oban.Job{args: %{"id" => media_item_id}}) do
-    media_item = Media.get_media_item!(media_item_id)
-
-    FilesystemHelpers.compute_and_save_media_filesize(media_item)
-
-    # Don't retry on failure - if it didn't work immediately there's no
-    # reason to believe it will work later.
-    :ok
-  end
-end

lib/pinchflat/filesystem/filesystem_helpers.ex

@@ -48,4 +48,36 @@ defmodule Pinchflat.Filesystem.FilesystemHelpers do
         err
     end
   end
+
+  @doc """
+  Deletes a file and removes any empty directories in the path.
+  Does NOT remove any directories that are not empty.
+
+  Returns :ok | {:error, any()}
+  """
+  def delete_file_and_remove_empty_directories(filepath) do
+    case File.rm(filepath) do
+      :ok ->
+        filepath
+        |> Path.dirname()
+        |> recursively_delete_empty_directories()
+
+      err ->
+        err
+    end
+  end
+
+  defp recursively_delete_empty_directories(directory) do
+    case File.rmdir(directory) do
+      :ok ->
+        directory
+        |> Path.dirname()
+        |> recursively_delete_empty_directories()
+
+      err ->
+        err
+    end
+
+    :ok
+  end
 end

lib/pinchflat/media/media.ex

@@ -7,9 +7,10 @@ defmodule Pinchflat.Media do
 
   alias Pinchflat.Repo
   alias Pinchflat.Tasks
-  alias Pinchflat.Media.MediaItem
   alias Pinchflat.Sources.Source
+  alias Pinchflat.Media.MediaItem
   alias Pinchflat.Metadata.MediaMetadata
+  alias Pinchflat.Filesystem.FilesystemHelpers
 
   @doc """
   Returns the list of media_items.
@@ -141,42 +142,6 @@ defmodule Pinchflat.Media do
   """
   def get_media_item!(id), do: Repo.get!(MediaItem, id)
 
-  @doc """
-  Produces a flat list of the filesystem paths for a media_item's downloaded files
-
-  NOTE: this can almost certainly be made private
-
-  Returns [binary()]
-  """
-  def media_filepaths(media_item) do
-    mapped_struct = Map.from_struct(media_item)
-
-    MediaItem.filepath_attributes()
-    |> Enum.map(fn
-      :subtitle_filepaths = field -> Enum.map(mapped_struct[field], fn [_, filepath] -> filepath end)
-      field -> List.wrap(mapped_struct[field])
-    end)
-    |> List.flatten()
-    |> Enum.filter(&is_binary/1)
-  end
-
-  @doc """
-  Produces a flat list of the filesystem paths for a media_item's metadata files.
-  Returns an empty list if the media_item has no metadata.
-
-  NOTE: this can almost certainly be made private
-
-  Returns [binary()] | []
-  """
-  def metadata_filepaths(media_item) do
-    metadata = Repo.preload(media_item, :metadata).metadata || %MediaMetadata{}
-    mapped_struct = Map.from_struct(metadata)
-
-    MediaMetadata.filepath_attributes()
-    |> Enum.map(fn field -> mapped_struct[field] end)
-    |> Enum.filter(&is_binary/1)
-  end
-
   @doc """
   Creates a media_item.
 
@@ -223,19 +188,20 @@ defmodule Pinchflat.Media do
   end
 
   @doc """
-  Deletes a media_item and its associated tasks.
+  Deletes a media_item, its associated tasks, and our internal metadata files.
-  Can optionally delete the media_item's files.
+  Can optionally delete the media_item's media files (media, thumbnail, subtitles, etc).
 
   Returns {:ok, %MediaItem{}} | {:error, %Ecto.Changeset{}}
   """
   def delete_media_item(%MediaItem{} = media_item, opts \\ []) do
     delete_files = Keyword.get(opts, :delete_files, false)
 
-    # NOTE: this should delete metadata no matter what
     if delete_files do
-      {:ok, _} = delete_all_attachments(media_item)
+      {:ok, _} = delete_media_files(media_item)
     end
 
+    # Should delete these no matter what
+    delete_internal_metadata_files(media_item)
     Tasks.delete_tasks_for(media_item)
     Repo.delete(media_item)
   end
@@ -247,27 +213,31 @@ defmodule Pinchflat.Media do
     MediaItem.changeset(media_item, attrs)
   end
 
-  # NOTE: refactor this
+  defp delete_media_files(media_item) do
-  defp delete_all_attachments(media_item) do
+    mapped_struct = Map.from_struct(media_item)
-    media_item = Repo.preload(media_item, :metadata)
 
-    media_item
+    MediaItem.filepath_attributes()
-    |> media_filepaths()
+    |> Enum.map(fn
-    |> Enum.concat(metadata_filepaths(media_item))
+      :subtitle_filepaths = field -> Enum.map(mapped_struct[field], fn [_, filepath] -> filepath end)
-    |> Enum.each(&File.rm/1)
+      field -> List.wrap(mapped_struct[field])
-
+    end)
-    # rmdir will attempt to delete the directory, but only if it is empty
+    |> List.flatten()
-    if media_item.media_filepath do
+    |> Enum.filter(&is_binary/1)
-      File.rmdir(Path.dirname(media_item.media_filepath))
+    |> Enum.each(&FilesystemHelpers.delete_file_and_remove_empty_directories/1)
-    end
-
-    if media_item.metadata && media_item.metadata.metadata_filepath do
-      File.rmdir(Path.dirname(media_item.metadata.metadata_filepath))
-    end
 
     {:ok, media_item}
   end
 
+  defp delete_internal_metadata_files(media_item) do
+    metadata = Repo.preload(media_item, :metadata).metadata || %MediaMetadata{}
+    mapped_struct = Map.from_struct(metadata)
+
+    MediaMetadata.filepath_attributes()
+    |> Enum.map(fn field -> mapped_struct[field] end)
+    |> Enum.filter(&is_binary/1)
+    |> Enum.each(&FilesystemHelpers.delete_file_and_remove_empty_directories/1)
+  end
+
   defp maybe_apply_cutoff_date(source) do
     if source.download_cutoff_date do
       dynamic([mi], mi.upload_date >= ^source.download_cutoff_date)

lib/pinchflat/metadata/source_metadata_storage_worker.ex

@@ -4,7 +4,10 @@ defmodule Pinchflat.Metadata.SourceMetadataStorageWorker do
   use Oban.Worker,
     queue: :remote_metadata,
     tags: ["media_source", "source_metadata", "remote_metadata"],
-    max_attempts: 1
+    max_attempts: 1,
+    # This is the only thing stopping this job from calling itself
+    # in an infinite loop.
+    unique: [period: 600]
 
   alias __MODULE__
   alias Pinchflat.Repo
@@ -16,27 +19,27 @@ defmodule Pinchflat.Metadata.SourceMetadataStorageWorker do
   @doc """
   Starts the source metadata storage worker and creates a task for the source.
 
-  IDEA: testing out this method of handling job kickoff. I think I like it, so
-  I may use it in other places. Just testing it for now
-
   Returns {:ok, %Task{}} | {:error, :duplicate_job} | {:error, %Ecto.Changeset{}}
   """
-  def kickoff_with_task(source) do
+  def kickoff_with_task(source, opts \\ []) do
     %{id: source.id}
-    |> SourceMetadataStorageWorker.new()
+    |> SourceMetadataStorageWorker.new(opts)
     |> Tasks.create_job_with_task(source)
   end
 
-  @impl Oban.Worker
   @doc """
   Fetches and stores metadata for a source in the secret metadata location.
 
   Returns :ok
   """
+  @impl Oban.Worker
   def perform(%Oban.Job{args: %{"id" => source_id}}) do
     source = Repo.preload(Sources.get_source!(source_id), :metadata)
     {:ok, metadata} = MediaCollection.get_source_metadata(source.original_url)
 
+    # Since updating a source kicks this job off again, we enforce job uniqueness (above)
+    # to once, per source, per x minutes. This is to prevent a job from calling itself
+    # in an infinite loop.
     Sources.update_source(source, %{
       metadata: %{
         metadata_filepath: MetadataFileHelpers.compress_and_store_metadata_for(source, metadata)

lib/pinchflat/slow_indexing/media_collection_indexing_worker.ex

@@ -13,7 +13,17 @@ defmodule Pinchflat.SlowIndexing.MediaCollectionIndexingWorker do
   alias Pinchflat.FastIndexing.FastIndexingWorker
   alias Pinchflat.SlowIndexing.SlowIndexingHelpers
 
-  @impl Oban.Worker
+  @doc """
+  Starts the source slow indexing worker and creates a task for the source.
+
+  Returns {:ok, %Task{}} | {:error, :duplicate_job} | {:error, %Ecto.Changeset{}}
+  """
+  def kickoff_with_task(source, opts \\ []) do
+    %{id: source.id}
+    |> MediaCollectionIndexingWorker.new(opts)
+    |> Tasks.create_job_with_task(source)
+  end
+
   @doc """
   The ID is that of a source _record_, not a YouTube channel/playlist ID. Indexes
   the provided source, kicks off downloads for each new MediaItem, and
@@ -58,6 +68,7 @@ defmodule Pinchflat.SlowIndexing.MediaCollectionIndexingWorker do
 
   Returns :ok | {:ok, %Task{}}
   """
+  @impl Oban.Worker
   def perform(%Oban.Job{args: %{"id" => source_id}}) do
     source = Sources.get_source!(source_id)
 

lib/pinchflat/slow_indexing/slow_indexing_helpers.ex

@@ -31,10 +31,7 @@ defmodule Pinchflat.SlowIndexing.SlowIndexingHelpers do
     Tasks.delete_pending_tasks_for(source, "MediaIndexingWorker")
     Tasks.delete_pending_tasks_for(source, "MediaCollectionIndexingWorker")
 
-    %{id: source.id}
+    MediaCollectionIndexingWorker.kickoff_with_task(source)
-    # Schedule this one immediately, but future ones will be on an interval
-    |> MediaCollectionIndexingWorker.new()
-    |> Tasks.create_job_with_task(source)
   end
 
   @doc """
@@ -125,9 +122,7 @@ defmodule Pinchflat.SlowIndexing.SlowIndexingHelpers do
         if source.download_media && Media.pending_download?(media_item) do
           Logger.debug("FileFollowerServer Handler: Enqueuing download task for #{inspect(media_attrs)}")
 
-          %{id: media_item.id}
+          MediaDownloadWorker.kickoff_with_task(media_item)
-          |> MediaDownloadWorker.new()
-          |> Tasks.create_job_with_task(media_item)
         end
 
       {:error, changeset} ->

lib/pinchflat/sources/sources.ex

@@ -12,8 +12,9 @@ defmodule Pinchflat.Sources do
   alias Pinchflat.Profiles.MediaProfile
   alias Pinchflat.YtDlp.MediaCollection
   alias Pinchflat.Metadata.SourceMetadata
+  alias Pinchflat.Filesystem.FilesystemHelpers
   alias Pinchflat.Downloading.DownloadingHelpers
-  alias Pinchflat.FastIndexing.FastIndexingHelpers
+  alias Pinchflat.FastIndexing.FastIndexingWorker
   alias Pinchflat.SlowIndexing.SlowIndexingHelpers
   alias Pinchflat.Metadata.SourceMetadataStorageWorker
 
@@ -108,8 +109,8 @@ defmodule Pinchflat.Sources do
       Media.delete_media_item(media_item, delete_files: delete_files)
     end)
 
-    Tasks.delete_tasks_for(source)
     delete_source_metadata_files(source)
+    Tasks.delete_tasks_for(source)
     Repo.delete(source)
   end
 
@@ -120,17 +121,9 @@ defmodule Pinchflat.Sources do
     Source.changeset(source, attrs, validation_stage)
   end
 
-  @doc """
+  # NOTE: When operating in the ideal path, this effectively adds an API call
-  Returns an `%Ecto.Changeset{}` for tracking source changes and additionally
+  # to the source creation/update process. Should be used only when needed.
-  fetches source details from the original_url (if provided). If the source
+  defp maybe_change_source_from_url(%Source{} = source, attrs) do
-  details cannot be fetched, an error is added to the changeset.
-
-  NOTE: When operating in the ideal path, this effectively adds an API call
-  to the source creation/update process. Should be used only when needed.
-
-  NOTE: this can almost certainly be made private now
-  """
-  def maybe_change_source_from_url(%Source{} = source, attrs) do
     case change_source(source, attrs) do
       %Ecto.Changeset{changes: %{original_url: _}} = changeset ->
         add_source_details_to_changeset(source, changeset)
@@ -149,7 +142,7 @@ defmodule Pinchflat.Sources do
       |> Enum.map(fn field -> mapped_struct[field] end)
       |> Enum.filter(&is_binary/1)
 
-    Enum.each(filepaths, &File.rm/1)
+    Enum.each(filepaths, &FilesystemHelpers.delete_file_and_remove_empty_directories/1)
   end
 
   defp add_source_details_to_changeset(source, changeset) do
@@ -270,7 +263,8 @@ defmodule Pinchflat.Sources do
   defp maybe_update_fast_indexing_task(changeset, source) do
     case changeset.changes do
       %{fast_index: true} ->
-        FastIndexingHelpers.kickoff_fast_indexing_task(source)
+        Tasks.delete_pending_tasks_for(source, "FastIndexingWorker")
+        FastIndexingWorker.kickoff_with_task(source)
 
       %{fast_index: false} ->
         Tasks.delete_pending_tasks_for(source, "FastIndexingWorker")

lib/pinchflat/tasks/tasks.ex

@@ -20,13 +20,17 @@ defmodule Pinchflat.Tasks do
   Returns the list of tasks for a given record type and ID. Optionally allows you to specify
   which worker or job states to include.
 
-  IDEA: this should be updated to take a struct instead of a record type and ID
-
   Returns [%Task{}, ...]
   """
-  def list_tasks_for(attached_record_type, attached_record_id, worker_name \\ nil, job_states \\ Oban.Job.states()) do
+  def list_tasks_for(record, worker_name \\ nil, job_states \\ Oban.Job.states()) do
     stringified_states = Enum.map(job_states, &to_string/1)
 
+    record_type =
+      case record do
+        %Source{} -> :source_id
+        %MediaItem{} -> :media_item_id
+      end
+
     worker_name_finder =
       if worker_name do
         # Workers are the full module name - we want to match on the string ENDING with
@@ -43,7 +47,7 @@ defmodule Pinchflat.Tasks do
     Repo.all(
       from t in Task,
         join: j in assoc(t, :job),
-        where: field(t, ^attached_record_type) == ^attached_record_id,
+        where: field(t, ^record_type) == ^record.id,
         where: ^worker_name_finder,
         where: j.state in ^stringified_states
     )
@@ -55,10 +59,9 @@ defmodule Pinchflat.Tasks do
 
   Returns [%Task{}, ...]
   """
-  def list_pending_tasks_for(attached_record_type, attached_record_id, worker_name \\ nil) do
+  def list_pending_tasks_for(record, worker_name \\ nil) do
     list_tasks_for(
-      attached_record_type,
+      record,
-      attached_record_id,
       worker_name,
       [:available, :scheduled, :retryable]
     )
@@ -128,14 +131,10 @@ defmodule Pinchflat.Tasks do
 
   Returns :ok
   """
-  def delete_tasks_for(attached_record, worker_name \\ nil) do
+  def delete_tasks_for(record, worker_name \\ nil) do
-    tasks =
+    record
-      case attached_record do
+    |> list_tasks_for(worker_name)
-        %Source{} = source -> list_tasks_for(:source_id, source.id, worker_name)
+    |> Enum.each(&delete_task/1)
-        %MediaItem{} = media_item -> list_tasks_for(:media_item_id, media_item.id, worker_name)
-      end
-
-    Enum.each(tasks, &delete_task/1)
   end
 
   @doc """
@@ -144,14 +143,10 @@ defmodule Pinchflat.Tasks do
 
   Returns :ok
   """
-  def delete_pending_tasks_for(attached_record, worker_name \\ nil) do
+  def delete_pending_tasks_for(record, worker_name \\ nil) do
-    tasks =
+    record
-      case attached_record do
+    |> list_pending_tasks_for(worker_name)
-        %Source{} = source -> list_pending_tasks_for(:source_id, source.id, worker_name)
+    |> Enum.each(&delete_task/1)
-        %MediaItem{} = media_item -> list_pending_tasks_for(:media_item_id, media_item.id, worker_name)
-      end
-
-    Enum.each(tasks, &delete_task/1)
   end
 
   @doc """

lib/pinchflat/yt_dlp/command_runner.ex

@@ -37,7 +37,8 @@ defmodule Pinchflat.YtDlp.CommandRunner do
 
     case System.cmd(command, formatted_command_opts, stderr_to_stdout: true) do
       {_, 0} ->
-        # IDEA: consider deleting the file after reading it
+        # IDEA: consider deleting the file after reading it. It's in the tmp dir, so it's not
+        # a huge deal, but it's still a good idea to clean up after ourselves.
         # (even on error? especially on error?)
         File.read(output_filepath)
 

lib/pinchflat/yt_dlp/media_collection.ex

@@ -6,7 +6,6 @@ defmodule Pinchflat.YtDlp.MediaCollection do
 
   require Logger
 
-  alias Pinchflat.Utils.FunctionUtils
   alias Pinchflat.Filesystem.FilesystemHelpers
   alias Pinchflat.YtDlp.Media, as: YtDlpMedia
 
@@ -36,11 +35,20 @@ defmodule Pinchflat.YtDlp.MediaCollection do
 
     case runner.run(url, command_opts, output_template, output_filepath: output_filepath) do
       {:ok, output} ->
-        output
+        parsed_lines =
-        |> String.split("\n", trim: true)
+          output
-        |> Enum.map(&Phoenix.json_library().decode!/1)
+          |> String.split("\n", trim: true)
-        |> Enum.map(&YtDlpMedia.response_to_struct/1)
+          |> Enum.map(fn line ->
-        |> FunctionUtils.wrap_ok()
+            case Phoenix.json_library().decode(line) do
+              {:ok, parsed_json} ->
+                YtDlpMedia.response_to_struct(parsed_json)
+
+              _ ->
+                nil
+            end
+          end)
+
+        {:ok, Enum.filter(parsed_lines, &(&1 != nil))}
 
       res ->
         res

lib/pinchflat_web/controllers/sources/source_controller.ex

@@ -46,7 +46,7 @@ defmodule PinchflatWeb.Sources.SourceController do
   def show(conn, %{"id" => id}) do
     source = Repo.preload(Sources.get_source!(id), :media_profile)
 
-    pending_tasks = Repo.preload(Tasks.list_pending_tasks_for(:source_id, source.id), :job)
+    pending_tasks = Repo.preload(Tasks.list_pending_tasks_for(source), :job)
     pending_media = Media.list_pending_media_items_for(source, limit: 100)
     downloaded_media = Media.list_downloaded_media_items_for(source, limit: 100)
 

mix.exs

@@ -13,6 +13,16 @@ defmodule Pinchflat.MixProject do
       preferred_cli_env: [
         check: :test,
         credo: :test
+      ],
+      test_coverage: [
+        ignore_modules: [
+          Pinchflat.HTTP.HTTPClient,
+          PinchflatWeb.Layouts,
+          Pinchflat.DataCase,
+          Pinchflat.Release,
+          ~r/Fixtures/,
+          ~r/HTML$/
+        ]
       ]
     ]
   end

test/pinchflat/boot/data_backfill_worker_test.exs

@@ -4,7 +4,7 @@ defmodule Pinchflat.Boot.DataBackfillWorkerTest do
   import Pinchflat.MediaFixtures
 
   alias Pinchflat.Boot.DataBackfillWorker
-  alias Pinchflat.Filesystem.FilesystemDataWorker
+  alias Pinchflat.JobFixtures.TestJobWorker
 
   describe "cancel_pending_backfill_jobs/0" do
     test "cancels all pending backfill jobs" do
@@ -21,14 +21,14 @@ defmodule Pinchflat.Boot.DataBackfillWorkerTest do
 
     test "does not cancel jobs for other workers" do
       %{id: 0}
-      |> FilesystemDataWorker.new()
+      |> TestJobWorker.new()
       |> Repo.insert_unique_job()
 
-      assert_enqueued(worker: FilesystemDataWorker)
+      assert_enqueued(worker: TestJobWorker)
 
       DataBackfillWorker.cancel_pending_backfill_jobs()
 
-      assert_enqueued(worker: FilesystemDataWorker)
+      assert_enqueued(worker: TestJobWorker)
     end
   end
 

test/pinchflat/downloading/download_option_builder_test.exs

@@ -197,12 +197,19 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilderTest do
   end
 
   describe "build/1 when testing quality options" do
-    test "it includes quality options", %{media_item: media_item} do
+    test "it includes quality options" do
-      media_item = update_media_profile_attribute(media_item, %{preferred_resolution: :"1080p"})
+      resolutions = ["360", "480", "720", "1080", "2160"]
 
-      assert {:ok, res} = DownloadOptionBuilder.build(media_item)
+      Enum.each(resolutions, fn resolution ->
+        resolution_atom = String.to_existing_atom(resolution <> "p")
 
-      assert {:format_sort, "res:1080,+codec:avc:m4a"} in res
+        media_profile = media_profile_fixture(%{preferred_resolution: resolution_atom})
+        source = source_fixture(%{media_profile_id: media_profile.id})
+        media_item = Repo.preload(media_item_fixture(source_id: source.id), source: :media_profile)
+
+        assert {:ok, res} = DownloadOptionBuilder.build(media_item)
+        assert {:format_sort, "res:#{resolution},+codec:avc:m4a"} in res
+      end)
     end
 
     test "it includes quality options for audio only", %{media_item: media_item} do
@@ -218,10 +225,10 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilderTest do
   defp update_media_profile_attribute(media_item_with_preloads, attrs) do
     media_item_with_preloads.source.media_profile
     |> Profiles.change_media_profile(attrs)
-    |> Repo.update!()
+    |> Repo.update()
 
     media_item_with_preloads
     |> Repo.reload()
-    |> Repo.preload(source: :media_profile)
+    |> Repo.preload([source: :media_profile], force: true)
   end
 end

test/pinchflat/downloading/downloading_helpers_test.exs

@@ -34,11 +34,11 @@ defmodule Pinchflat.Downloading.DownloadingHelpersTest do
       source = source_fixture()
       media_item = media_item_fixture(source_id: source.id, media_filepath: nil)
 
-      assert [] = Tasks.list_tasks_for(:media_item_id, media_item.id)
+      assert [] = Tasks.list_tasks_for(media_item)
 
       assert :ok = DownloadingHelpers.enqueue_pending_download_tasks(source)
 
-      assert [_] = Tasks.list_tasks_for(:media_item_id, media_item.id)
+      assert [_] = Tasks.list_tasks_for(media_item)
     end
 
     test "it does not create a job if the source is set to not download" do
@@ -54,7 +54,7 @@ defmodule Pinchflat.Downloading.DownloadingHelpersTest do
       media_item = media_item_fixture(source_id: source.id, media_filepath: nil)
 
       assert :ok = DownloadingHelpers.enqueue_pending_download_tasks(source)
-      assert [] = Tasks.list_tasks_for(:media_item_id, media_item.id)
+      assert [] = Tasks.list_tasks_for(media_item)
     end
   end
 
@@ -69,7 +69,7 @@ defmodule Pinchflat.Downloading.DownloadingHelpersTest do
       assert :ok = DownloadingHelpers.dequeue_pending_download_tasks(source)
 
       refute_enqueued(worker: MediaDownloadWorker)
-      assert [] = Tasks.list_tasks_for(:media_item_id, media_item.id)
+      assert [] = Tasks.list_tasks_for(media_item)
     end
   end
 end

test/pinchflat/downloading/media_download_worker_test.exs

@@ -5,25 +5,37 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do
   import Pinchflat.MediaFixtures
 
   alias Pinchflat.Sources
+  alias Pinchflat.Filesystem.FilesystemHelpers
   alias Pinchflat.Downloading.MediaDownloadWorker
-  alias Pinchflat.Filesystem.FilesystemDataWorker
 
   setup :verify_on_exit!
 
   setup do
-    media_item =
-      Repo.preload(
-        media_item_fixture(%{media_filepath: nil}),
-        [:metadata, source: :media_profile]
-      )
-
     stub(HTTPClientMock, :get, fn _url, _headers, _opts ->
       {:ok, ""}
     end)
 
+    media_item =
+      %{media_filepath: nil}
+      |> media_item_fixture()
+      |> Repo.preload([:metadata, source: :media_profile])
+
     {:ok, %{media_item: media_item}}
   end
 
+  describe "kickoff_with_task/2" do
+    test "starts the worker", %{media_item: media_item} do
+      assert [] = all_enqueued(worker: MediaDownloadWorker)
+      assert {:ok, _} = MediaDownloadWorker.kickoff_with_task(media_item)
+      assert [_] = all_enqueued(worker: MediaDownloadWorker)
+    end
+
+    test "attaches a task", %{media_item: media_item} do
+      assert {:ok, task} = MediaDownloadWorker.kickoff_with_task(media_item)
+      assert task.media_item_id == media_item.id
+    end
+  end
+
   describe "perform/1" do
     test "it saves attributes to the media_item", %{media_item: media_item} do
       expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot ->
@@ -70,16 +82,18 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do
       perform_job(MediaDownloadWorker, %{id: media_item.id})
     end
 
-    test "it schedules a filesystem data worker", %{media_item: media_item} do
+    test "it saves the file's size to the database", %{media_item: media_item} do
       expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot ->
-        {:ok, render_metadata(:media_metadata)}
+        metadata = render_parsed_metadata(:media_metadata)
+        FilesystemHelpers.write_p!(metadata["filepath"], "test")
+
+        {:ok, Phoenix.json_library().encode!(metadata)}
       end)
 
-      assert [] = all_enqueued(worker: FilesystemDataWorker)
-
       perform_job(MediaDownloadWorker, %{id: media_item.id})
+      media_item = Repo.reload(media_item)
 
-      assert [_] = all_enqueued(worker: FilesystemDataWorker)
+      assert media_item.media_size_bytes > 0
     end
   end
 end

test/pinchflat/fast_indexing/fast_indexing_helpers_test.exs

@@ -2,51 +2,20 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpersTest do
   use Pinchflat.DataCase
 
   import Mox
-  import Pinchflat.TasksFixtures
   import Pinchflat.MediaFixtures
   import Pinchflat.SourcesFixtures
   import Pinchflat.ProfilesFixtures
 
   alias Pinchflat.Tasks
-  alias Pinchflat.Tasks.Task
   alias Pinchflat.Media.MediaItem
   alias Pinchflat.Downloading.MediaDownloadWorker
   alias Pinchflat.FastIndexing.MediaIndexingWorker
   alias Pinchflat.FastIndexing.FastIndexingHelpers
-  alias Pinchflat.FastIndexing.FastIndexingWorker
 
   setup :verify_on_exit!
 
   @media_url "https://www.youtube.com/watch?v=test_1"
 
-  describe "kickoff_fast_indexing_task/1" do
-    test "it schedules a job" do
-      source = source_fixture()
-
-      assert {:ok, _} = FastIndexingHelpers.kickoff_fast_indexing_task(source)
-
-      assert_enqueued(worker: FastIndexingWorker, args: %{"id" => source.id})
-    end
-
-    test "it creates and attaches a task" do
-      source = source_fixture()
-
-      assert {:ok, %Task{} = task} = FastIndexingHelpers.kickoff_fast_indexing_task(source)
-
-      assert task.source_id == source.id
-    end
-
-    test "it deletes any fast indexing tasks for the source" do
-      source = source_fixture()
-      {:ok, job} = Oban.insert(FastIndexingWorker.new(%{"id" => source.id}))
-      task = task_fixture(source_id: source.id, job_id: job.id)
-
-      assert {:ok, _} = FastIndexingHelpers.kickoff_fast_indexing_task(source)
-
-      assert_raise Ecto.NoResultsError, fn -> Repo.reload!(task) end
-    end
-  end
-
   describe "kickoff_indexing_tasks_from_youtube_rss_feed/1" do
     setup do
       {:ok, [source: source_fixture()]}
@@ -104,7 +73,7 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpersTest do
     test "creates a download task record", %{source: source} do
       assert {:ok, media_item} = FastIndexingHelpers.index_and_enqueue_download_for_media_item(source, @media_url)
 
-      assert [_] = Tasks.list_tasks_for(:media_item_id, media_item.id, "MediaDownloadWorker")
+      assert [_] = Tasks.list_tasks_for(media_item, "MediaDownloadWorker")
     end
 
     test "does not enqueue a download job if the source does not allow it" do

test/pinchflat/fast_indexing/fast_indexing_worker_test.exs

@@ -9,6 +9,23 @@ defmodule Pinchflat.FastIndexing.FastIndexingWorkerTest do
 
   setup :verify_on_exit!
 
+  describe "kickoff_with_task/2" do
+    test "starts the worker" do
+      source = source_fixture(fast_index: true)
+
+      assert [] = all_enqueued(worker: FastIndexingWorker)
+      assert {:ok, _} = FastIndexingWorker.kickoff_with_task(source)
+      assert [_] = all_enqueued(worker: FastIndexingWorker)
+    end
+
+    test "attaches a task" do
+      source = source_fixture(fast_index: true)
+
+      assert {:ok, task} = FastIndexingWorker.kickoff_with_task(source)
+      assert task.source_id == source.id
+    end
+  end
+
   describe "perform/1" do
     test "calls out to Youtube RSS if enabled" do
       expect(HTTPClientMock, :get, fn _url -> {:ok, ""} end)
@@ -29,6 +46,16 @@ defmodule Pinchflat.FastIndexing.FastIndexingWorkerTest do
       )
     end
 
+    test "does not reschedule if that would create a duplicate job" do
+      stub(HTTPClientMock, :get, fn _url -> {:ok, ""} end)
+      source = source_fixture(fast_index: true)
+
+      perform_job(FastIndexingWorker, %{"id" => source.id})
+      perform_job(FastIndexingWorker, %{"id" => source.id})
+
+      assert [_] = all_enqueued(worker: FastIndexingWorker)
+    end
+
     test "does not call out to Youtube RSS if disabled" do
       expect(HTTPClientMock, :get, 0, fn _url -> {:ok, ""} end)
       source = source_fixture(fast_index: false)

test/pinchflat/fast_indexing/media_indexing_worker_test.exs

@@ -6,8 +6,8 @@ defmodule Pinchflat.FastIndexing.MediaIndexingWorkerTest do
   import Pinchflat.SourcesFixtures
 
   alias Pinchflat.Media.MediaItem
-  alias Pinchflat.FastIndexing.MediaIndexingWorker
   alias Pinchflat.Downloading.MediaDownloadWorker
+  alias Pinchflat.FastIndexing.MediaIndexingWorker
 
   @media_url "https://www.youtube.com/watch?v=1234567890"
 
@@ -19,6 +19,19 @@ defmodule Pinchflat.FastIndexing.MediaIndexingWorkerTest do
     {:ok, source: source}
   end
 
+  describe "kickoff_with_task/2" do
+    test "starts the worker", %{source: source} do
+      assert [] = all_enqueued(worker: MediaIndexingWorker)
+      assert {:ok, _} = MediaIndexingWorker.kickoff_with_task(source, @media_url)
+      assert [_] = all_enqueued(worker: MediaIndexingWorker)
+    end
+
+    test "attaches a task", %{source: source} do
+      assert {:ok, task} = MediaIndexingWorker.kickoff_with_task(source, @media_url)
+      assert task.source_id == source.id
+    end
+  end
+
   describe "perform/1" do
     test "indexes the media item and saves it to the database", %{source: source} do
       expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot ->

test/pinchflat/filesystem/filesystem_data_worker_test.exs

@@ -1,19 +0,0 @@
-defmodule Pinchflat.Filesystem.FilesystemDataWorkerTest do
-  use Pinchflat.DataCase
-
-  import Pinchflat.MediaFixtures
-
-  alias Pinchflat.Filesystem.FilesystemDataWorker
-
-  describe "perform/1" do
-    test "Computes and stores the media file size" do
-      media_item = media_item_with_attachments()
-
-      refute media_item.media_size_bytes
-
-      perform_job(FilesystemDataWorker, %{id: media_item.id})
-
-      assert Repo.reload!(media_item).media_size_bytes
-    end
-  end
-end

test/pinchflat/filesystem/filesystem_helpers_test.exs

@@ -56,4 +56,55 @@ defmodule Pinchflat.Filesystem.FilesystemHelpersTest do
       File.rm!(filepath)
     end
   end
+
+  describe "delete_file_and_remove_empty_directories/1" do
+    test "deletes file at the provided filepath" do
+      filepath = FilesystemHelpers.generate_metadata_tmpfile(:json)
+
+      assert File.exists?(filepath)
+
+      assert :ok = FilesystemHelpers.delete_file_and_remove_empty_directories(filepath)
+
+      refute File.exists?(filepath)
+    end
+
+    test "deletes empty directories" do
+      tmpfile_directory = Application.get_env(:pinchflat, :tmpfile_directory)
+      filepath = Path.join([tmpfile_directory, "foo", "bar", "baz", "qux.json"])
+      FilesystemHelpers.write_p!(filepath, "")
+
+      assert :ok = FilesystemHelpers.delete_file_and_remove_empty_directories(filepath)
+
+      refute File.exists?(filepath)
+      refute File.exists?(Path.join([tmpfile_directory, "foo", "bar", "baz"]))
+      refute File.exists?(Path.join([tmpfile_directory, "foo", "bar"]))
+      refute File.exists?(Path.join([tmpfile_directory, "foo"]))
+    end
+
+    test "does not delete directories with other files in them" do
+      tmpfile_directory = Application.get_env(:pinchflat, :tmpfile_directory)
+      filepath_1 = Path.join([tmpfile_directory, "foo", "bar", "baz", "qux.json"])
+      filepath_2 = Path.join([tmpfile_directory, "foo", "baz.json"])
+      FilesystemHelpers.write_p!(filepath_1, "")
+      FilesystemHelpers.write_p!(filepath_2, "")
+
+      assert :ok = FilesystemHelpers.delete_file_and_remove_empty_directories(filepath_1)
+
+      refute File.exists?(filepath_1)
+      refute File.exists?(Path.join([tmpfile_directory, "foo", "bar", "baz"]))
+      refute File.exists?(Path.join([tmpfile_directory, "foo", "bar"]))
+
+      assert File.exists?(filepath_2)
+      assert File.exists?(Path.join([tmpfile_directory, "foo"]))
+
+      # cleanup
+      FilesystemHelpers.delete_file_and_remove_empty_directories(filepath_2)
+    end
+
+    test "returns an error if file could not be deleted" do
+      filepath = "/nonexistent/file.json"
+
+      assert {:error, _} = FilesystemHelpers.delete_file_and_remove_empty_directories(filepath)
+    end
+  end
 end

test/pinchflat/media_test.exs

@@ -360,57 +360,6 @@ defmodule Pinchflat.MediaTest do
     end
   end
 
-  describe "media_filepaths/1" do
-    test "returns filepaths in a flat list" do
-      filepaths = %{
-        media_filepath: "/video/test.mp4",
-        thumbnail_filepath: "/video/test.jpg",
-        subtitle_filepaths: [["en", "video/test.srt"]]
-      }
-
-      media_item = media_item_fixture(filepaths)
-
-      assert Media.media_filepaths(media_item) == [
-               "/video/test.mp4",
-               "/video/test.jpg",
-               "video/test.srt"
-             ]
-    end
-
-    test "strips out nil values" do
-      filepaths = %{
-        media_filepath: "/video/test.mp4",
-        thumbnail_filepath: nil,
-        subtitle_filepaths: [["en", nil]]
-      }
-
-      media_item = media_item_fixture(filepaths)
-
-      assert Media.media_filepaths(media_item) == ["/video/test.mp4"]
-    end
-  end
-
-  describe "metadata_filepaths" do
-    test "returns filepaths in a flat list" do
-      filepaths = %{
-        metadata_filepath: "/metadata.json.gz",
-        thumbnail_filepath: "/thumbnail.jpg"
-      }
-
-      media_item = media_item_fixture(%{metadata: filepaths})
-
-      assert Media.metadata_filepaths(media_item) == [
-               "/metadata.json.gz",
-               "/thumbnail.jpg"
-             ]
-    end
-
-    test "returns an empty list when there is no metadata" do
-      media_item = media_item_fixture()
-      assert Media.metadata_filepaths(media_item) == []
-    end
-  end
-
   describe "create_media_item/1" do
     test "creating with valid data creates a media_item" do
       valid_attrs = %{
@@ -515,6 +464,26 @@ defmodule Pinchflat.MediaTest do
       assert {:ok, _} = Media.delete_media_item(media_item)
       assert File.exists?(media_item.media_filepath)
     end
+
+    test "does delete the media item's metadata files" do
+      stub(HTTPClientMock, :get, fn _url, _headers, _opts -> {:ok, ""} end)
+      media_item = Repo.preload(media_item_with_attachments(), :metadata)
+
+      update_attrs = %{
+        metadata: %{
+          metadata_filepath: MetadataFileHelpers.compress_and_store_metadata_for(media_item, %{}),
+          thumbnail_filepath:
+            MetadataFileHelpers.download_and_store_thumbnail_for(media_item, %{
+              "thumbnail" => "https://example.com/thumbnail.jpg"
+            })
+        }
+      }
+
+      {:ok, updated_media_item} = Media.update_media_item(media_item, update_attrs)
+
+      assert {:ok, _} = Media.delete_media_item(updated_media_item)
+      refute File.exists?(updated_media_item.metadata.metadata_filepath)
+    end
   end
 
   describe "delete_media_item/2 when testing file deletion" do

test/pinchflat/metadata/source_metadata_storage_worker_test.exs

@@ -51,5 +51,28 @@ defmodule Pinchflat.Metadata.SourceMetadataStorageWorkerTest do
 
       assert metadata == %{"title" => "test"}
     end
+
+    test "won't call itself in an infinite loop" do
+      stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:ok, "{}"} end)
+      source = source_fixture()
+
+      perform_job(SourceMetadataStorageWorker, %{id: source.id})
+      perform_job(SourceMetadataStorageWorker, %{id: source.id})
+
+      assert [_] = all_enqueued(worker: SourceMetadataStorageWorker)
+    end
+
+    test "doesn't prevent over source jobs from running" do
+      stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:ok, "{}"} end)
+      source_1 = source_fixture()
+      source_2 = source_fixture()
+
+      perform_job(SourceMetadataStorageWorker, %{id: source_1.id})
+      perform_job(SourceMetadataStorageWorker, %{id: source_1.id})
+      perform_job(SourceMetadataStorageWorker, %{id: source_2.id})
+      perform_job(SourceMetadataStorageWorker, %{id: source_2.id})
+
+      assert [_, _] = all_enqueued(worker: SourceMetadataStorageWorker)
+    end
   end
 end

test/pinchflat/slow_indexing/media_collection_indexing_worker_test.exs

@@ -103,7 +103,7 @@ defmodule Pinchflat.SlowIndexing.MediaCollectionIndexingWorkerTest do
       source = source_fixture(index_frequency_minutes: 10)
 
       task_count_fetcher = fn ->
-        Enum.count(Tasks.list_tasks_for(:source_id, source.id, "MediaCollectionIndexingWorker"))
+        Enum.count(Tasks.list_tasks_for(source, "MediaCollectionIndexingWorker"))
       end
 
       assert_changed([from: 0, to: 1], task_count_fetcher, fn ->

test/pinchflat/slow_indexing/slow_indexing_helpers_test.exs

@@ -149,7 +149,7 @@ defmodule Pinchflat.SlowIndexing.SlowIndexingHelpersTest do
 
       SlowIndexingHelpers.index_and_enqueue_download_for_media_items(source)
 
-      assert [] = Tasks.list_tasks_for(:media_item_id, media_item.id)
+      assert [] = Tasks.list_tasks_for(media_item)
     end
 
     test "it doesn't blow up if a media item cannot be coerced into a struct", %{source: source} do
@@ -290,5 +290,22 @@ defmodule Pinchflat.SlowIndexing.SlowIndexingHelpersTest do
       assert Repo.aggregate(MediaItem, :count, :id) == 3
       assert [_, _, _] = all_enqueued(worker: MediaDownloadWorker)
     end
+
+    test "does not blow up if the file returns invalid json", %{source: source} do
+      watcher_poll_interval = Application.get_env(:pinchflat, :file_watcher_poll_interval)
+
+      stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot, addl_opts ->
+        filepath = Keyword.get(addl_opts, :output_filepath)
+        File.write(filepath, "INVALID")
+
+        # Need to add a delay to ensure the file watcher has time to read the file
+        :timer.sleep(watcher_poll_interval * 2)
+        # We know we're testing the file watcher since the syncronous call will only
+        # return an empty string (creating no records)
+        {:ok, ""}
+      end)
+
+      assert [] = SlowIndexingHelpers.index_and_enqueue_download_for_media_items(source)
+    end
   end
 end

test/pinchflat/sources_test.exs

@@ -508,70 +508,6 @@ defmodule Pinchflat.SourcesTest do
     end
   end
 
-  describe "maybe_change_source_from_url/2" do
-    test "it returns a changeset" do
-      stub(YtDlpRunnerMock, :run, &channel_mock/3)
-      source = source_fixture()
-
-      assert %Ecto.Changeset{} = Sources.maybe_change_source_from_url(source, %{})
-    end
-
-    test "it does not fetch source details if the original_url isn't in the changeset" do
-      expect(YtDlpRunnerMock, :run, 0, &channel_mock/3)
-
-      changeset = Sources.maybe_change_source_from_url(%Source{}, %{name: "some updated name"})
-
-      assert %Ecto.Changeset{} = changeset
-    end
-
-    test "it fetches source details if the original_url is in the changeset" do
-      expect(YtDlpRunnerMock, :run, &channel_mock/3)
-
-      changeset =
-        Sources.maybe_change_source_from_url(%Source{}, %{
-          original_url: "https://www.youtube.com/channel/abc123"
-        })
-
-      assert %Ecto.Changeset{} = changeset
-    end
-
-    test "it adds source details to the changeset, keeping the orignal details" do
-      expect(YtDlpRunnerMock, :run, &channel_mock/3)
-
-      media_profile = media_profile_fixture()
-      media_profile_id = media_profile.id
-
-      changeset =
-        Sources.maybe_change_source_from_url(%Source{}, %{
-          original_url: "https://www.youtube.com/channel/abc123",
-          media_profile_id: media_profile.id
-        })
-
-      assert %Ecto.Changeset{} = changeset
-      assert String.starts_with?(changeset.changes.collection_id, "some_channel_id_")
-
-      assert %{
-               collection_name: "some channel name",
-               media_profile_id: ^media_profile_id,
-               original_url: "https://www.youtube.com/channel/abc123"
-             } = changeset.changes
-    end
-
-    test "it adds an error to the changeset if the runner fails" do
-      expect(YtDlpRunnerMock, :run, 1, fn _url, _opts, _ot ->
-        {:error, "some error", 1}
-      end)
-
-      changeset =
-        Sources.maybe_change_source_from_url(%Source{}, %{
-          original_url: "https://www.youtube.com/channel/abc123"
-        })
-
-      assert %Ecto.Changeset{} = changeset
-      assert errors_on(changeset).original_url == ["could not fetch source details from URL"]
-    end
-  end
-
   defp playlist_mock(_url, _opts, _ot) do
     {
       :ok,

test/pinchflat/tasks_test.exs

@@ -36,53 +36,59 @@ defmodule Pinchflat.TasksTest do
     end
   end
 
-  describe "list_tasks_for/4" do
+  describe "list_tasks_for/3" do
     test "it lets you specify which record type/ID to join on" do
-      task = task_fixture()
+      source = source_fixture()
+      task = task_fixture(source_id: source.id)
 
-      assert Tasks.list_tasks_for(:source_id, task.source_id) == [task]
+      assert Tasks.list_tasks_for(source, nil, [:available]) == [task]
     end
 
     test "it lets you specify which job states to include" do
-      task = task_fixture()
+      source = source_fixture()
+      task = task_fixture(source_id: source.id)
 
-      assert Tasks.list_tasks_for(:source_id, task.source_id, nil, [:available]) == [task]
+      assert Tasks.list_tasks_for(source, nil, [:available]) == [task]
-      assert Tasks.list_tasks_for(:source_id, task.source_id, nil, [:cancelled]) == []
+      assert Tasks.list_tasks_for(source, nil, [:cancelled]) == []
     end
 
     test "it lets you specify which worker to include" do
-      task = task_fixture()
+      source = source_fixture()
+      task = task_fixture(source_id: source.id)
 
-      assert Tasks.list_tasks_for(:source_id, task.source_id, "TestJobWorker") == [task]
+      assert Tasks.list_tasks_for(source, "TestJobWorker") == [task]
-      assert Tasks.list_tasks_for(:source_id, task.source_id, "FooBarWorker") == []
+      assert Tasks.list_tasks_for(source, "FooBarWorker") == []
     end
 
     test "it includes all workers if no worker is specified" do
-      task = task_fixture()
+      source = source_fixture()
+      task = task_fixture(source_id: source.id)
 
-      assert Tasks.list_tasks_for(:source_id, task.source_id, nil) == [task]
+      assert Tasks.list_tasks_for(source, nil) == [task]
     end
   end
 
   describe "list_pending_tasks_for/3" do
     test "it lists pending tasks" do
-      task = task_fixture()
+      source = source_fixture()
+      task = task_fixture(source_id: source.id)
 
-      assert Tasks.list_pending_tasks_for(:source_id, task.source_id) == [task]
+      assert Tasks.list_pending_tasks_for(source) == [task]
     end
 
     test "it does not list non-pending tasks" do
-      task = Repo.preload(task_fixture(), :job)
+      task = Repo.preload(task_fixture(), [:job, :source])
       :ok = Oban.cancel_job(task.job)
 
-      assert Tasks.list_pending_tasks_for(:source_id, task.source_id) == []
+      assert Tasks.list_pending_tasks_for(task.source) == []
     end
 
     test "it lets you specify which worker to include" do
-      task = task_fixture()
+      source = source_fixture()
+      task = task_fixture(source_id: source.id)
 
-      assert Tasks.list_pending_tasks_for(:source_id, task.source_id, "TestJobWorker") == [task]
+      assert Tasks.list_pending_tasks_for(source, "TestJobWorker") == [task]
-      assert Tasks.list_pending_tasks_for(:source_id, task.source_id, "FooBarWorker") == []
+      assert Tasks.list_pending_tasks_for(source, "FooBarWorker") == []
     end
   end
 

test/pinchflat/yt_dlp/media_collection_test.exs

@@ -62,6 +62,15 @@ defmodule Pinchflat.YtDlp.MediaCollectionTest do
       assert_receive {:handler, filename}
       assert String.ends_with?(filename, ".json")
     end
+
+    test "gracefully handles partially failed responses" do
+      expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl_opts ->
+        {:ok, "INVALID\n\n" <> source_attributes_return_fixture() <> "\nINVALID\n"}
+      end)
+
+      assert {:ok, [%Media{media_id: "video1"}, %Media{media_id: "video2"}, %Media{media_id: "video3"}]} =
+               MediaCollection.get_media_attributes_for_collection(@channel_url)
+    end
   end
 
   describe "get_source_details/1" do