Flaky test fixes

lib/philomena/application.ex

@@ -24,6 +24,9 @@ defmodule Philomena.Application do
       # Mailer
       {Task.Supervisor, name: Philomena.AsyncEmailSupervisor},
 
+      # Image upload processing
+      {Task.Supervisor, name: Philomena.ImageUploadSupervisor},
+
       # Starts a worker by calling: Philomena.Worker.start_link(arg)
       # {Philomena.Worker, arg},
       {Redix, name: :redix, host: Application.get_env(:philomena, :redis_host)},

lib/philomena/images.ex

@@ -401,8 +401,8 @@ defmodule Philomena.Images do
   defp sources_for_edit(sources), do: sources
 
   defp async_upload(image, upload) do
-    linked_pid =
-      spawn(fn ->
+    {:ok, upload_pid} =
+      Task.Supervisor.start_child(Philomena.ImageUploadSupervisor, fn ->
         # Make sure task will finish before VM exit
         Process.flag(:trap_exit, true)
 
@@ -416,12 +416,12 @@ defmodule Philomena.Images do
       end)
 
     # Give the upload to the linked process
-    Plug.Upload.give_away(upload.path, linked_pid, self())
+    Plug.Upload.give_away(upload.path, upload_pid, self())
 
     # Free up the linked process
-    send(linked_pid, :ready)
+    send(upload_pid, :ready)
 
-    linked_pid
+    :ok
   end
 
   defp try_upload(image, retry_count) when retry_count < 100 do
@@ -1477,14 +1477,14 @@ defmodule Philomena.Images do
   ## Examples
 
       iex> create_image(actor, %{"tag_input" => "safe"}, upload)
-      {:ok, %{image: %Image{}, upload_pid: pid}}
+      {:ok, %Image{}}
 
       iex> create_image(banned_actor, params, upload)
       {:error, :ban}
 
   """
   @spec create_image(Actor.t(), map() | nil, PhilomenaMedia.Upload.t() | nil) ::
-          {:ok, image_upload()}
+          {:ok, Image.t()}
           | {:error, :ban | :unauthorized | :rate_limited | Ecto.Changeset.t()}
   def create_image(%Actor{user: user} = actor, params, upload) do
     with :ok <- verify_write_access(actor),
@@ -1536,17 +1536,13 @@ defmodule Philomena.Images do
       |> Multi.transact_with_automatic_retry()
       |> case do
         {:ok, %{image: %Image{} = image}} ->
-          upload_pid = async_upload(image, upload)
+          :ok = async_upload(image, upload)
 
           image = Repo.preload(image, tags: :aliases)
 
           broadcast_image_create(image)
 
-          # Return the upload PID along with the created image so that the caller
-          # can control the lifecycle of the upload if needed. It's useful, for
-          # example for the seeding process to know when to delete the temp file
-          # used for uploading.
-          {:ok, %{image: image, upload_pid: upload_pid}}
+          {:ok, image}
 
         {:error, :action_reservation, :rate_limited, _changes} ->
           {:error, :rate_limited}
@@ -1560,16 +1556,6 @@ defmodule Philomena.Images do
     end
   end
 
-  @typedoc """
-  Result of the `upload_image/3` function. The image was created in the DB but an
-  upload process could still be running in the background with its PID given in the
-  `upload_pid` field.
-  """
-  @type image_upload :: %{
-          image: %Image{},
-          upload_pid: pid
-        }
-
   @doc group: "Moderation and lifecycle"
   @doc """
   Returns the paginated approval queue for `actor`: unapproved images, oldest

lib/philomena_web/controllers/api/json/image_controller.ex

@@ -27,7 +27,7 @@ defmodule PhilomenaWeb.Api.Json.ImageController do
     upload = PhilomenaMedia.Upload.cast(image_params, "image")
 
     case Images.create_image(conn.assigns.actor, image_params, upload) do
-      {:ok, %{image: image}} ->
+      {:ok, image} ->
         render(conn, "show.json", image: image, interactions: [])
 
       {:error, %Ecto.Changeset{} = changeset} ->

lib/philomena_web/controllers/image_controller.ex

@@ -92,7 +92,7 @@ defmodule PhilomenaWeb.ImageController do
     upload = PhilomenaMedia.Upload.cast(params["image"], "image")
 
     case Images.create_image(conn.assigns.actor, params["image"], upload) do
-      {:ok, %{image: image}} ->
+      {:ok, image} ->
         conn
         |> put_flash(:info, "Image created successfully.")
         |> redirect(to: ~p"/images/#{image}")

priv/repo/seeds_development.exs

@@ -84,7 +84,7 @@ for image_def <- resources["remote_images"] do
     upload
   )
   |> case do
-    {:ok, %{image: image}} ->
+    {:ok, image} ->
       Images.create_image_approve(admin_actor, image.id)
 
       IO.puts("Created image ##{image.id}")

test/philomena/comments_test.exs

@@ -1019,7 +1019,9 @@ defmodule Philomena.CommentsTest do
     test "a valid anonymous fingerprinted actor creates a comment with no author",
          %{image: image} do
       assert {:ok, %Comment{} = comment} =
-               Comments.create_comment(actor(nil), image.id, %{"body" => "An anonymous comment"})
+               Comments.create_comment(actor(nil, ip: random_ip()), image.id, %{
+                 "body" => "An anonymous comment"
+               })
 
       assert comment.user_id == nil
       assert comment.body == "An anonymous comment"

test/philomena/images_test.exs

@@ -1,9 +1,11 @@
 defmodule Philomena.ImagesTest do
-  use Philomena.DataCase, async: true
+  # async: false - successful image uploads spawn an upload process
+  # (Images.async_upload/2) that hits the Repo; it is only allowed on the
+  # sandbox connection in shared mode, which is enabled for sync tests.
+  use Philomena.DataCase, async: false
 
   import Ecto.Query
 
-  alias Ecto.Adapters.SQL.Sandbox
   alias Philomena.Events
   alias Philomena.Multi
   alias Philomena.ImageFaves
@@ -30,6 +32,7 @@ defmodule Philomena.ImagesTest do
   alias PhilomenaQuery.Search
   alias PhilomenaQuery.SearchHelpers
 
+  import Philomena.AsyncUpload
   import Philomena.GalleriesFixtures
   import Philomena.FiltersFixtures
   import Philomena.ImagesFixtures
@@ -4357,26 +4360,6 @@ defmodule Philomena.ImagesTest do
     |> DateTime.truncate(:second)
   end
 
-  # Waits for the background upload process a successful upload spawns to exit.
-  # It shares the test process's sandbox connection, so letting it outlive the
-  # test leaves it retrying against a dead owner. The spawned process is our
-  # direct child.
-  defp await_async_upload do
-    test_pid = self()
-
-    for pid <- Process.list(), Process.info(pid, :parent) == {:parent, test_pid} do
-      ref = Process.monitor(pid)
-
-      receive do
-        {:DOWN, ^ref, :process, ^pid, _reason} -> :ok
-      after
-        5_000 -> raise "async upload process #{inspect(pid)} did not exit"
-      end
-    end
-
-    :ok
-  end
-
   describe "show_image/2" do
     test "an anonymous viewer loads a visible image with zero change counts" do
       image = image_fixture()
@@ -4694,22 +4677,21 @@ defmodule Philomena.ImagesTest do
   end
 
   describe "create_image/3" do
+    setup do
+      allow_async_uploads()
+    end
+
     test "a normal actor uploads an image and the row exists" do
       actor = actor(confirmed_user_fixture())
       :ok = Events.subscribe_events()
 
-      assert {:ok, %{image: %Image{} = image, upload_pid: pid}} =
+      assert {:ok, %Image{} = image} =
                Images.create_image(
                  actor,
                  %{"tag_input" => "safe, solo, pony"},
                  media_png_upload()
                )
 
-      # The background upload process finishes the persist/repair work against
-      # the Repo; in an async case it owns no sandbox connection, so grant it the
-      # test's before awaiting its exit.
-      Sandbox.allow(Repo, self(), pid)
-
       assert Repo.get(Image, image.id)
       assert source_urls(image) == []
 
@@ -4722,7 +4704,7 @@ defmodule Philomena.ImagesTest do
       actor = actor(confirmed_user_fixture())
       sources = ["https://example.com/first", "https://example.com/second"]
 
-      assert {:ok, %{image: image, upload_pid: pid}} =
+      assert {:ok, image} =
                Images.create_image(
                  actor,
                  %{
@@ -4737,15 +4719,14 @@ defmodule Philomena.ImagesTest do
                  media_png_upload()
                )
 
-      Sandbox.allow(Repo, self(), pid)
       assert source_urls(image) == Enum.sort(sources)
       await_async_upload()
     end
 
     test "ignores blank source rows during upload" do
       actor = actor(confirmed_user_fixture())
 
-      assert {:ok, %{image: image, upload_pid: pid}} =
+      assert {:ok, image} =
                Images.create_image(
                  actor,
                  %{
@@ -4759,7 +4740,6 @@ defmodule Philomena.ImagesTest do
                  media_png_upload()
                )
 
-      Sandbox.allow(Repo, self(), pid)
       assert source_urls(image) == ["https://example.com/source"]
       await_async_upload()
     end
@@ -4790,7 +4770,7 @@ defmodule Philomena.ImagesTest do
       |> Ecto.Changeset.change(images_count: 4)
       |> Repo.update!()
 
-      assert {:ok, %{image: image, upload_pid: pid}} =
+      assert {:ok, image} =
                Images.create_image(
                  actor(user),
                  %{"tag_input" => "safe, solo, pony"},
@@ -4805,7 +4785,6 @@ defmodule Philomena.ImagesTest do
 
       assert user_id == user.id
 
-      Sandbox.allow(Repo, self(), pid)
       await_async_upload()
     end
 
@@ -4847,7 +4826,7 @@ defmodule Philomena.ImagesTest do
       actor = actor(confirmed_user_fixture())
       track_rate_limit(actor, :image_create)
 
-      assert {:ok, %{image: %Image{}, upload_pid: pid}} =
+      assert {:ok, %Image{}} =
                Images.create_image(
                  actor,
                  %{"tag_input" => "safe, solo, pony"},
@@ -4857,9 +4836,6 @@ defmodule Philomena.ImagesTest do
       # Recording happens synchronously once create_image succeeds.
       assert rate_limit_count(actor, :image_create) == "1"
 
-      # Let the background upload process finish against the test's sandbox
-      # connection before the test exits.
-      Sandbox.allow(Repo, self(), pid)
       await_async_upload()
     end
 

test/philomena/posts_test.exs

@@ -826,7 +826,7 @@ defmodule Philomena.PostsTest do
       # and reaches the public forum/topic create; the engine records the post
       # with a nil user (anonymous attribution).
       assert {:ok, %Post{} = post} =
-               Posts.create_post(actor(nil), forum.short_name, topic.slug, %{
+               Posts.create_post(actor(nil, ip: random_ip()), forum.short_name, topic.slug, %{
                  "body" => "An anonymous reply"
                })
 

test/philomena_web/controllers/api/json/image_controller_test.exs

@@ -4,6 +4,7 @@ defmodule PhilomenaWeb.Api.Json.ImageControllerTest do
   # sandbox connection in shared mode, which ConnCase enables for sync tests.
   use PhilomenaWeb.ConnCase, async: false
 
+  import Philomena.AsyncUpload
   import Philomena.ImagesFixtures
   import Philomena.UsersFixtures
 
@@ -14,6 +15,10 @@ defmodule PhilomenaWeb.Api.Json.ImageControllerTest do
 
   @png_fixture Path.absname("test/support/fixtures/files/upload-test.png")
 
+  setup_all do
+    allow_async_uploads()
+  end
+
   describe "GET /api/v1/json/images/:id" do
     test "shows an image with the full representation set", %{conn: conn} do
       image = image_fixture(sources: ["https://example.com/art/1", "https://example.com/art/2"])

test/philomena_web/controllers/image_controller_test.exs

@@ -3,6 +3,7 @@ defmodule PhilomenaWeb.ImageControllerTest do
 
   @moduletag :search
 
+  import Philomena.AsyncUpload
   import Philomena.CommentsFixtures
   import Philomena.ImagesFixtures
   import Philomena.UsersFixtures
@@ -14,6 +15,10 @@ defmodule PhilomenaWeb.ImageControllerTest do
   alias Philomena.Repo
   alias Philomena.Roles.Role
 
+  setup_all do
+    allow_async_uploads()
+  end
+
   setup do
     Search.clear_index!(Image)
     # :show and :new render the quick tag table, which queries the tags index

test/support/async_upload.ex

@@ -0,0 +1,36 @@
+defmodule Philomena.AsyncUpload do
+  @moduledoc """
+  Helpers for managing the async image-upload lifecycle from tests.
+  """
+
+  @doc """
+  Allows supervised image upload tasks to use the current test's sandbox
+  connection.
+  """
+  def allow_async_uploads do
+    Ecto.Adapters.SQL.Sandbox.allow(
+      Philomena.Repo,
+      self(),
+      Philomena.ImageUploadSupervisor
+    )
+
+    :ok
+  end
+
+  @doc """
+  Waits for all supervised image upload tasks to exit.
+  """
+  def await_async_upload do
+    for pid <- Task.Supervisor.children(Philomena.ImageUploadSupervisor) do
+      ref = Process.monitor(pid)
+
+      receive do
+        {:DOWN, ^ref, :process, ^pid, _reason} -> :ok
+      after
+        5_000 -> raise "async upload process #{inspect(pid)} did not exit"
+      end
+    end
+
+    :ok
+  end
+end

test/support/conn_case.ex

@@ -225,33 +225,6 @@ defmodule PhilomenaWeb.ConnCase do
     %{conn | remote_ip: {10, rem(div(n, 65536), 256), rem(div(n, 256), 256), rem(n, 256)}}
   end
 
-  @doc """
-  Waits for the background upload process spawned by a successful image
-  `:create` to exit.
-
-  A successful `:create` has `Philomena.Images.create_image/2` spawn an
-  unsupervised upload process that writes to the Repo. Its sandbox allowance
-  dies with the test process, so wait for it to exit before the test ends;
-  otherwise it retries with `OwnershipError` every 5s for the rest of the
-  suite. The endpoint call runs in the test process, so the upload process is
-  our direct child.
-  """
-  def await_async_upload do
-    test_pid = self()
-
-    for pid <- Process.list(), Process.info(pid, :parent) == {:parent, test_pid} do
-      ref = Process.monitor(pid)
-
-      receive do
-        {:DOWN, ^ref, :process, ^pid, _reason} -> :ok
-      after
-        5_000 -> raise "async upload process #{inspect(pid)} did not exit"
-      end
-    end
-
-    :ok
-  end
-
   @doc """
   Helper to set up the conn with global assigns set by the application shell.
   """