Fix records not getting indexed because pubsub is faster than databas…

…e transaction commit (#995) * Fix records not getting indexed because pubsub is faster than database transaction commit
ambry-app · Feb 15, 2025 · 4c80015 · 4c80015
1 parent 7d851b1
commit 4c80015
Show file tree

Hide file tree

Showing 10 changed files with 49 additions and 46 deletions.
diff --git a/compose.yml b/compose.yml
@@ -12,7 +12,6 @@ services:
     image: ghcr.io/ambry-app/firefox-headless-marionette:latest
     ports:
       - 2828:2828
-    restart: unless-stopped
 
 volumes:
   pgdata:
diff --git a/lib/ambry/books.ex b/lib/ambry/books.ex
@@ -148,12 +148,11 @@ defmodule Ambry.Books do
 
   """
   def delete_book(%Book{} = book) do
-    Repo.transact(fn ->
+    fn ->
       case Repo.delete(change_book(book)) do
         {:ok, book} ->
           maybe_delete_image(book.image_path)
-          PubSub.broadcast_delete(book)
-          :ok
+          {:ok, book}
 
         {:error, changeset} ->
           if Keyword.has_key?(changeset.errors, :media) do
@@ -162,7 +161,9 @@ defmodule Ambry.Books do
             {:error, changeset}
           end
       end
-    end)
+    end
+    |> Repo.transact()
+    |> tap_ok(&PubSub.broadcast_delete/1)
   end
 
   defp maybe_delete_image(nil), do: :noop
@@ -376,12 +377,11 @@ defmodule Ambry.Books do
       {:error, %Ecto.Changeset{}}
   """
   def delete_series(%Series{} = series) do
-    Repo.transact(fn ->
-      with {:ok, series} <- Repo.delete(change_series(series)) do
-        PubSub.broadcast_delete(series)
-        :ok
-      end
-    end)
+    fn ->
+      Repo.delete(change_series(series))
+    end
+    |> Repo.transact()
+    |> tap_ok(&PubSub.broadcast_delete/1)
   end
 
   @doc """

diff --git a/lib/ambry/media.ex b/lib/ambry/media.ex
@@ -173,14 +173,15 @@ defmodule Ambry.Media do
 
   """
   def update_media(%Media{} = media, attrs, for: action) do
-    Repo.transact(fn ->
+    fn ->
       media
       |> Media.changeset(attrs, for: action)
       |> tap(&maybe_generate_thumbnails(&1, media))
       |> Repo.update()
-      |> tap_ok(&PubSub.broadcast_update/1)
-      |> tap_ok(&maybe_maybe_delete_image(&1, media))
-    end)
+    end
+    |> Repo.transact()
+    |> tap_ok(&PubSub.broadcast_update/1)
+    |> tap_ok(&maybe_maybe_delete_image(&1, media))
   end
 
   defp maybe_generate_thumbnails(changeset, media) do
@@ -210,14 +211,15 @@ defmodule Ambry.Media do
 
   """
   def delete_media(%Media{} = media) do
-    Repo.transact(fn ->
+    fn ->
       case Repo.delete(media) do
         {:ok, media} ->
           delete_media_files(media)
-          PubSub.broadcast_delete(media)
-          :ok
+          {:ok, media}
       end
-    end)
+    end
+    |> Repo.transact()
+    |> tap_ok(&PubSub.broadcast_delete/1)
   end
 
   defp delete_media_files(%Media{} = media) do

diff --git a/lib/ambry/people.ex b/lib/ambry/people.ex
@@ -121,7 +121,7 @@ defmodule Ambry.People do
 
   """
   def create_person(attrs \\ %{}) do
-    Repo.transact(fn ->
+    fn ->
       %Person{}
       |> Person.changeset(attrs)
       |> Repo.insert()
@@ -132,8 +132,9 @@ defmodule Ambry.People do
           |> Oban.insert!()
         end
       end)
-      |> tap_ok(&PubSub.broadcast_create/1)
-    end)
+    end
+    |> Repo.transact()
+    |> tap_ok(&PubSub.broadcast_create/1)
   end
 
   @doc """
@@ -149,7 +150,7 @@ defmodule Ambry.People do
 
   """
   def update_person(%Person{} = person, attrs) do
-    Repo.transact(fn ->
+    fn ->
       person
       |> Repo.preload(@person_direct_assoc_preloads)
       |> Person.changeset(attrs)
@@ -161,12 +162,13 @@ defmodule Ambry.People do
         end
       end)
       |> Repo.update()
-      |> tap_ok(&PubSub.broadcast_update/1)
-      |> tap_ok(fn updated_person ->
-        if is_nil(updated_person.image_path) && !is_nil(person.image_path) do
-          maybe_delete_image(person.image_path)
-        end
-      end)
+    end
+    |> Repo.transact()
+    |> tap_ok(&PubSub.broadcast_update/1)
+    |> tap_ok(fn updated_person ->
+      if is_nil(updated_person.image_path) && !is_nil(person.image_path) do
+        maybe_delete_image(person.image_path)
+      end
     end)
   end
 
@@ -197,7 +199,6 @@ defmodule Ambry.People do
           if !is_nil(person.thumbnails),
             do: Thumbnails.try_delete_thumbnails(person.thumbnails)
 
-          PubSub.broadcast_delete(person)
           {:ok, person}
 
         {:error, changeset} ->
@@ -214,6 +215,7 @@ defmodule Ambry.People do
       end
     end
     |> Repo.transact()
+    |> tap_ok(&PubSub.broadcast_delete/1)
     |> handle_delete_result()
   end
 

diff --git a/lib/ambry_web/live/admin/book_live/index.ex b/lib/ambry_web/live/admin/book_live/index.ex
@@ -82,7 +82,7 @@ defmodule AmbryWeb.Admin.BookLive.Index do
     book = Books.get_book!(id)
 
     case Books.delete_book(book) do
-      :ok ->
+      {:ok, _book} ->
         {:noreply,
          socket
          |> refresh_books()

diff --git a/lib/ambry_web/live/admin/media_live/form/audible_import_form.ex b/lib/ambry_web/live/admin/media_live/form/audible_import_form.ex
@@ -171,8 +171,8 @@ defmodule AmbryWeb.Admin.MediaLive.Form.AudibleImportForm do
 
   defp select_book(book) do
     matching_narrators =
-      Enum.map(book.narrators, fn author ->
-        Search.find_first(author.name, Person)
+      Enum.map(book.narrators, fn narrator ->
+        Search.find_first(narrator.name, Person)
       end)
 
     %{selected_book: book, matching_narrators: matching_narrators}

diff --git a/lib/ambry_web/live/admin/media_live/index.ex b/lib/ambry_web/live/admin/media_live/index.ex
@@ -85,7 +85,7 @@ defmodule AmbryWeb.Admin.MediaLive.Index do
   @impl Phoenix.LiveView
   def handle_event("delete", %{"id" => id}, socket) do
     media = Media.get_media!(id)
-    :ok = Media.delete_media(media)
+    {:ok, _media} = Media.delete_media(media)
 
     {:noreply, refresh_media(socket)}
   end

diff --git a/lib/ambry_web/live/admin/series_live/index.ex b/lib/ambry_web/live/admin/series_live/index.ex
@@ -78,7 +78,7 @@ defmodule AmbryWeb.Admin.SeriesLive.Index do
   @impl Phoenix.LiveView
   def handle_event("delete", %{"id" => id}, socket) do
     series = Books.get_series!(id)
-    :ok = Books.delete_series(series)
+    {:ok, _series} = Books.delete_series(series)
 
     {:noreply,
      socket

diff --git a/test/ambry/books_test.exs b/test/ambry/books_test.exs
@@ -235,7 +235,7 @@ defmodule Ambry.BooksTest do
     test "deletes a book" do
       book = insert(:book, image_path: nil)
 
-      :ok = Books.delete_book(book)
+      {:ok, _book} = Books.delete_book(book)
 
       assert_raise Ecto.NoResultsError, fn ->
         Books.get_book!(book.id)
@@ -248,7 +248,7 @@ defmodule Ambry.BooksTest do
 
       assert File.exists?(Ambry.Paths.web_to_disk(book.image_path))
 
-      :ok = Books.delete_book(book)
+      {:ok, _book} = Books.delete_book(book)
 
       refute File.exists?(Ambry.Paths.web_to_disk(book.image_path))
     end
@@ -261,7 +261,7 @@ defmodule Ambry.BooksTest do
       assert File.exists?(Ambry.Paths.web_to_disk(book.image_path))
 
       fun = fn ->
-        :ok = Books.delete_book(book2)
+        {:ok, _book} = Books.delete_book(book2)
       end
 
       assert capture_log(fun) =~ "Not deleting file because it's still in use"
@@ -273,7 +273,7 @@ defmodule Ambry.BooksTest do
       book = insert(:book)
 
       fun = fn ->
-        :ok = Books.delete_book(book)
+        {:ok, _book} = Books.delete_book(book)
       end
 
       assert capture_log(fun) =~ "Couldn't delete file (enoent)"
@@ -535,7 +535,7 @@ defmodule Ambry.BooksTest do
     test "deletes a series" do
       series = insert(:series)
 
-      :ok = Books.delete_series(series)
+      {:ok, _series} = Books.delete_series(series)
 
       assert_raise Ecto.NoResultsError, fn ->
         Books.get_series!(series.id)

diff --git a/test/ambry/media_test.exs b/test/ambry/media_test.exs
@@ -384,7 +384,7 @@ defmodule Ambry.MediaTest do
           image_path: nil
         )
 
-      :ok = Media.delete_media(media)
+      {:ok, _media} = Media.delete_media(media)
 
       assert_raise Ecto.NoResultsError, fn ->
         Media.get_media!(media.id)
@@ -401,7 +401,7 @@ defmodule Ambry.MediaTest do
       assert media.hls_path |> Paths.web_to_disk() |> File.exists?()
       assert media.hls_path |> Paths.hls_playlist_path() |> Paths.web_to_disk() |> File.exists?()
 
-      :ok = Media.delete_media(media)
+      {:ok, _media} = Media.delete_media(media)
 
       refute File.dir?(media.source_path)
       refute media.mp4_path |> Paths.web_to_disk() |> File.exists?()
@@ -417,7 +417,7 @@ defmodule Ambry.MediaTest do
       media.mp4_path |> Paths.web_to_disk() |> File.rm!()
 
       fun = fn ->
-        :ok = Media.delete_media(media)
+        {:ok, _media} = Media.delete_media(media)
       end
 
       assert capture_log(fun) =~ "Couldn't delete file (enoent)"
@@ -429,7 +429,7 @@ defmodule Ambry.MediaTest do
 
       assert File.exists?(Ambry.Paths.web_to_disk(media.image_path))
 
-      :ok = Media.delete_media(media)
+      {:ok, _media} = Media.delete_media(media)
 
       refute File.exists?(Ambry.Paths.web_to_disk(media.image_path))
     end
@@ -442,7 +442,7 @@ defmodule Ambry.MediaTest do
       assert File.exists?(Ambry.Paths.web_to_disk(media.image_path))
 
       fun = fn ->
-        :ok = Media.delete_media(media2)
+        {:ok, _media} = Media.delete_media(media2)
       end
 
       assert capture_log(fun) =~ "Not deleting file because it's still in use"
@@ -454,7 +454,7 @@ defmodule Ambry.MediaTest do
       media = insert(:media)
 
       fun = fn ->
-        :ok = Media.delete_media(media)
+        {:ok, _media} = Media.delete_media(media)
       end
 
       assert capture_log(fun) =~ "Couldn't delete file (enoent)"