From 4dfa50f256ce7966ff127a12f91c9fdeabfff114 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 19 Jun 2024 21:25:24 -0400 Subject: [PATCH 1/3] Rename RichMediaExpirationWorker to RichMediaWorker --- lib/pleroma/web/rich_media/backfill.ex | 4 ++-- ...{rich_media_expiration_worker.ex => rich_media_worker.ex} | 4 ++-- .../web/rich_media/parser/ttl/aws_signed_url_test.exs | 5 ++++- test/pleroma/web/rich_media/parser/ttl/opengraph_test.exs | 5 ++++- 4 files changed, 12 insertions(+), 6 deletions(-) rename lib/pleroma/workers/{rich_media_expiration_worker.ex => rich_media_worker.ex} (71%) diff --git a/lib/pleroma/web/rich_media/backfill.ex b/lib/pleroma/web/rich_media/backfill.ex index 4ec50e132..46b879434 100644 --- a/lib/pleroma/web/rich_media/backfill.ex +++ b/lib/pleroma/web/rich_media/backfill.ex @@ -6,7 +6,7 @@ defmodule Pleroma.Web.RichMedia.Backfill do alias Pleroma.Web.RichMedia.Card alias Pleroma.Web.RichMedia.Parser alias Pleroma.Web.RichMedia.Parser.TTL - alias Pleroma.Workers.RichMediaExpirationWorker + alias Pleroma.Workers.RichMediaWorker require Logger @@ -72,7 +72,7 @@ defmodule Pleroma.Web.RichMedia.Backfill do {:ok, ttl} when is_number(ttl) -> timestamp = DateTime.from_unix!(ttl) - RichMediaExpirationWorker.new(%{"url" => url}, scheduled_at: timestamp) + RichMediaWorker.new(%{"op" => "expire", "url" => url}, scheduled_at: timestamp) |> Oban.insert() _ -> diff --git a/lib/pleroma/workers/rich_media_expiration_worker.ex b/lib/pleroma/workers/rich_media_worker.ex similarity index 71% rename from lib/pleroma/workers/rich_media_expiration_worker.ex rename to lib/pleroma/workers/rich_media_worker.ex index 0b74687cf..968395c64 100644 --- a/lib/pleroma/workers/rich_media_expiration_worker.ex +++ b/lib/pleroma/workers/rich_media_worker.ex @@ -2,14 +2,14 @@ # Copyright © 2017-2022 Pleroma Authors # SPDX-License-Identifier: AGPL-3.0-only -defmodule Pleroma.Workers.RichMediaExpirationWorker do +defmodule Pleroma.Workers.RichMediaWorker do alias Pleroma.Web.RichMedia.Card use Oban.Worker, queue: :background @impl Oban.Worker - def perform(%Job{args: %{"url" => url} = _args}) do + def perform(%Job{args: %{"op" => "expire", "url" => url} = _args}) do Card.delete(url) end end diff --git a/test/pleroma/web/rich_media/parser/ttl/aws_signed_url_test.exs b/test/pleroma/web/rich_media/parser/ttl/aws_signed_url_test.exs index cc28aa7f3..8fd9e6a5f 100644 --- a/test/pleroma/web/rich_media/parser/ttl/aws_signed_url_test.exs +++ b/test/pleroma/web/rich_media/parser/ttl/aws_signed_url_test.exs @@ -74,7 +74,10 @@ defmodule Pleroma.Web.RichMedia.Parser.TTL.AwsSignedUrlTest do Card.get_or_backfill_by_url(url) - assert_enqueued(worker: Pleroma.Workers.RichMediaExpirationWorker, args: %{"url" => url}) + assert_enqueued( + worker: Pleroma.Workers.RichMediaWorker, + args: %{"op" => "expire", "url" => url} + ) [%Oban.Job{scheduled_at: scheduled_at}] = all_enqueued() diff --git a/test/pleroma/web/rich_media/parser/ttl/opengraph_test.exs b/test/pleroma/web/rich_media/parser/ttl/opengraph_test.exs index 770968d47..e275ee523 100644 --- a/test/pleroma/web/rich_media/parser/ttl/opengraph_test.exs +++ b/test/pleroma/web/rich_media/parser/ttl/opengraph_test.exs @@ -36,6 +36,9 @@ defmodule Pleroma.Web.RichMedia.Parser.TTL.OpengraphTest do Card.get_or_backfill_by_url(url) - assert_enqueued(worker: Pleroma.Workers.RichMediaExpirationWorker, args: %{"url" => url}) + assert_enqueued( + worker: Pleroma.Workers.RichMediaWorker, + args: %{"op" => "expire", "url" => url} + ) end end From 17d04ccc8bcf3f0e033ff4333c6153edf904d4f8 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 19 Jun 2024 23:18:38 -0400 Subject: [PATCH 2/3] RichMedia backfill processing through Oban --- changelog.d/rich_media_backfill.change | 1 + config/test.exs | 2 - .../web/mastodon_api/views/status_view.ex | 9 +--- lib/pleroma/web/rich_media/backfill.ex | 45 ++++--------------- lib/pleroma/web/rich_media/card.ex | 13 +++--- lib/pleroma/workers/rich_media_worker.ex | 8 +++- .../chat_message_reference_view_test.exs | 3 ++ test/pleroma/web/rich_media/card_test.exs | 5 +++ .../parser/ttl/aws_signed_url_test.exs | 20 ++++++--- .../rich_media/parser/ttl/opengraph_test.exs | 13 ++++++ 10 files changed, 59 insertions(+), 60 deletions(-) create mode 100644 changelog.d/rich_media_backfill.change diff --git a/changelog.d/rich_media_backfill.change b/changelog.d/rich_media_backfill.change new file mode 100644 index 000000000..d746ac8ce --- /dev/null +++ b/changelog.d/rich_media_backfill.change @@ -0,0 +1 @@ +Rich Media backfilling is now an Oban job diff --git a/config/test.exs b/config/test.exs index 9afb2763a..1e8168548 100644 --- a/config/test.exs +++ b/config/test.exs @@ -180,8 +180,6 @@ config :pleroma, Pleroma.Application, config :pleroma, Pleroma.Web.Streaming, sync_streaming: true -config :pleroma, Pleroma.Web.MastodonAPI.StatusView, sync_fetching: true - config :pleroma, Pleroma.Uploaders.Uploader, timeout: 1_000 config :pleroma, Pleroma.Emoji.Loader, test_emoji: true diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex index 963658b1e..d9d7e516a 100644 --- a/lib/pleroma/web/mastodon_api/views/status_view.ex +++ b/lib/pleroma/web/mastodon_api/views/status_view.ex @@ -8,7 +8,6 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do require Pleroma.Constants alias Pleroma.Activity - alias Pleroma.Config alias Pleroma.HTML alias Pleroma.Maps alias Pleroma.Object @@ -31,13 +30,7 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do # pagination is restricted to 40 activities at a time defp fetch_rich_media_for_activities(activities) do Enum.each(activities, fn activity -> - fun = fn -> Card.get_by_activity(activity) end - - if Config.get([__MODULE__, :sync_fetching], false) do - fun.() - else - spawn(fun) - end + Card.get_by_activity(activity) end) end diff --git a/lib/pleroma/web/rich_media/backfill.ex b/lib/pleroma/web/rich_media/backfill.ex index 46b879434..1d8cc87d4 100644 --- a/lib/pleroma/web/rich_media/backfill.ex +++ b/lib/pleroma/web/rich_media/backfill.ex @@ -10,31 +10,21 @@ defmodule Pleroma.Web.RichMedia.Backfill do require Logger - @backfiller Pleroma.Config.get([__MODULE__, :provider], Pleroma.Web.RichMedia.Backfill.Task) @cachex Pleroma.Config.get([:cachex, :provider], Cachex) - @max_attempts 3 - @retry 5_000 - def start(%{url: url} = args) when is_binary(url) do + @spec run(map()) :: + :ok | {:error, {:invalid_metadata, any()} | :body_too_large | {:content, any()} | any()} + def run(%{"url" => url} = args) do url_hash = Card.url_to_hash(url) - args = - args - |> Map.put(:attempt, 1) - |> Map.put(:url_hash, url_hash) - - @backfiller.run(args) - end - - def run(%{url: url, url_hash: url_hash, attempt: attempt} = args) - when attempt <= @max_attempts do case Parser.parse(url) do {:ok, fields} -> {:ok, card} = Card.create(url, fields) maybe_schedule_expiration(url, fields) - if Map.has_key?(args, :activity_id) do + with %{"activity_id" => activity_id} <- args, + false <- is_nil(activity_id) do stream_update(args) end @@ -54,19 +44,10 @@ defmodule Pleroma.Web.RichMedia.Backfill do e -> Logger.debug("Rich media error for #{url}: #{inspect(e)}") - - :timer.sleep(@retry * attempt) - - run(%{args | attempt: attempt + 1}) + {:error, e} end end - def run(%{url: url, url_hash: url_hash}) do - Logger.debug("Rich media failure for #{url}") - - negative_cache(url_hash, :timer.minutes(15)) - end - defp maybe_schedule_expiration(url, fields) do case TTL.process(fields, url) do {:ok, ttl} when is_number(ttl) -> @@ -80,22 +61,14 @@ defmodule Pleroma.Web.RichMedia.Backfill do end end - defp stream_update(%{activity_id: activity_id}) do + defp stream_update(%{"activity_id" => activity_id}) do Pleroma.Activity.get_by_id(activity_id) |> Pleroma.Activity.normalize() |> Pleroma.Web.ActivityPub.ActivityPub.stream_out() end defp warm_cache(key, val), do: @cachex.put(:rich_media_cache, key, val) - defp negative_cache(key, ttl \\ nil), do: @cachex.put(:rich_media_cache, key, nil, ttl: ttl) -end -defmodule Pleroma.Web.RichMedia.Backfill.Task do - alias Pleroma.Web.RichMedia.Backfill - - def run(args) do - Task.Supervisor.start_child(Pleroma.TaskSupervisor, Backfill, :run, [args], - name: {:global, {:rich_media, args.url_hash}} - ) - end + defp negative_cache(key, ttl \\ :timer.minutes(15)), + do: @cachex.put(:rich_media_cache, key, nil, ttl: ttl) end diff --git a/lib/pleroma/web/rich_media/card.ex b/lib/pleroma/web/rich_media/card.ex index 040066f36..72ff5e791 100644 --- a/lib/pleroma/web/rich_media/card.ex +++ b/lib/pleroma/web/rich_media/card.ex @@ -7,8 +7,8 @@ defmodule Pleroma.Web.RichMedia.Card do alias Pleroma.HTML alias Pleroma.Object alias Pleroma.Repo - alias Pleroma.Web.RichMedia.Backfill alias Pleroma.Web.RichMedia.Parser + alias Pleroma.Workers.RichMediaWorker @cachex Pleroma.Config.get([:cachex, :provider], Cachex) @config_impl Application.compile_env(:pleroma, [__MODULE__, :config_impl], Pleroma.Config) @@ -75,17 +75,18 @@ defmodule Pleroma.Web.RichMedia.Card do def get_by_url(nil), do: nil - @spec get_or_backfill_by_url(String.t(), map()) :: t() | nil - def get_or_backfill_by_url(url, backfill_opts \\ %{}) do + @spec get_or_backfill_by_url(String.t(), keyword()) :: t() | nil + def get_or_backfill_by_url(url, opts \\ []) do if @config_impl.get([:rich_media, :enabled]) do case get_by_url(url) do %__MODULE__{} = card -> card nil -> - backfill_opts = Map.put(backfill_opts, :url, url) + activity_id = Keyword.get(opts, :activity, nil) - Backfill.start(backfill_opts) + RichMediaWorker.new(%{"op" => "backfill", "url" => url, "activity_id" => activity_id}) + |> Oban.insert() nil @@ -137,7 +138,7 @@ defmodule Pleroma.Web.RichMedia.Card do nil else {:cached, url} -> - get_or_backfill_by_url(url, %{activity_id: activity.id}) + get_or_backfill_by_url(url, activity_id: activity.id) _ -> :error diff --git a/lib/pleroma/workers/rich_media_worker.ex b/lib/pleroma/workers/rich_media_worker.ex index 968395c64..f18ac658a 100644 --- a/lib/pleroma/workers/rich_media_worker.ex +++ b/lib/pleroma/workers/rich_media_worker.ex @@ -3,13 +3,17 @@ # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Workers.RichMediaWorker do + alias Pleroma.Web.RichMedia.Backfill alias Pleroma.Web.RichMedia.Card - use Oban.Worker, - queue: :background + use Oban.Worker, queue: :background, max_attempts: 3, unique: [period: 300] @impl Oban.Worker def perform(%Job{args: %{"op" => "expire", "url" => url} = _args}) do Card.delete(url) end + + def perform(%Job{args: %{"op" => "backfill", "url" => _url} = args}) do + Backfill.run(args) + end end diff --git a/test/pleroma/web/pleroma_api/views/chat_message_reference_view_test.exs b/test/pleroma/web/pleroma_api/views/chat_message_reference_view_test.exs index f17add774..c78c03aba 100644 --- a/test/pleroma/web/pleroma_api/views/chat_message_reference_view_test.exs +++ b/test/pleroma/web/pleroma_api/views/chat_message_reference_view_test.exs @@ -9,6 +9,7 @@ defmodule Pleroma.Web.PleromaAPI.ChatMessageReferenceViewTest do alias Pleroma.Chat alias Pleroma.Chat.MessageReference alias Pleroma.Object + alias Pleroma.Tests.ObanHelpers alias Pleroma.UnstubbedConfigMock, as: ConfigMock alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.Web.CommonAPI @@ -70,6 +71,8 @@ defmodule Pleroma.Web.PleromaAPI.ChatMessageReferenceViewTest do media_id: upload.id ) + ObanHelpers.perform_all() + object = Object.normalize(activity, fetch: false) cm_ref = MessageReference.for_chat_and_object(chat, object) diff --git a/test/pleroma/web/rich_media/card_test.exs b/test/pleroma/web/rich_media/card_test.exs index 516ac9951..c76df99e2 100644 --- a/test/pleroma/web/rich_media/card_test.exs +++ b/test/pleroma/web/rich_media/card_test.exs @@ -5,6 +5,7 @@ defmodule Pleroma.Web.RichMedia.CardTest do use Pleroma.DataCase, async: true + alias Pleroma.Tests.ObanHelpers alias Pleroma.UnstubbedConfigMock, as: ConfigMock alias Pleroma.Web.CommonAPI alias Pleroma.Web.RichMedia.Card @@ -36,6 +37,8 @@ defmodule Pleroma.Web.RichMedia.CardTest do content_type: "text/markdown" }) + ObanHelpers.perform_all() + assert %Card{url_hash: ^url_hash, fields: _} = Card.get_by_activity(activity) end @@ -50,6 +53,7 @@ defmodule Pleroma.Web.RichMedia.CardTest do # Force a backfill Card.get_by_activity(activity) + ObanHelpers.perform_all() assert match?( %Card{url_hash: ^original_url_hash, fields: _}, @@ -62,6 +66,7 @@ defmodule Pleroma.Web.RichMedia.CardTest do # Force a backfill Card.get_by_activity(activity) + ObanHelpers.perform_all() assert match?( %Card{url_hash: ^updated_url_hash, fields: _}, diff --git a/test/pleroma/web/rich_media/parser/ttl/aws_signed_url_test.exs b/test/pleroma/web/rich_media/parser/ttl/aws_signed_url_test.exs index 8fd9e6a5f..e02dd437a 100644 --- a/test/pleroma/web/rich_media/parser/ttl/aws_signed_url_test.exs +++ b/test/pleroma/web/rich_media/parser/ttl/aws_signed_url_test.exs @@ -4,10 +4,11 @@ defmodule Pleroma.Web.RichMedia.Parser.TTL.AwsSignedUrlTest do use Pleroma.DataCase, async: false - use Oban.Testing, repo: Pleroma.Repo + use Oban.Testing, repo: Pleroma.Repo, testing: :inline import Mox + alias Pleroma.Tests.ObanHelpers alias Pleroma.UnstubbedConfigMock, as: ConfigMock alias Pleroma.Web.RichMedia.Card alias Pleroma.Web.RichMedia.Parser.TTL.AwsSignedUrl @@ -74,12 +75,19 @@ defmodule Pleroma.Web.RichMedia.Parser.TTL.AwsSignedUrlTest do Card.get_or_backfill_by_url(url) - assert_enqueued( - worker: Pleroma.Workers.RichMediaWorker, - args: %{"op" => "expire", "url" => url} - ) + # Find the backfill job + expected_job = + [ + worker: "Pleroma.Workers.RichMediaWorker", + args: %{"op" => "backfill", "url" => url} + ] - [%Oban.Job{scheduled_at: scheduled_at}] = all_enqueued() + assert_enqueued(expected_job) + + # Run it manually + ObanHelpers.perform_all() + + [%Oban.Job{scheduled_at: scheduled_at} | _] = all_enqueued() timestamp_dt = Timex.parse!(timestamp, "{ISO:Basic:Z}") diff --git a/test/pleroma/web/rich_media/parser/ttl/opengraph_test.exs b/test/pleroma/web/rich_media/parser/ttl/opengraph_test.exs index e275ee523..6805e786d 100644 --- a/test/pleroma/web/rich_media/parser/ttl/opengraph_test.exs +++ b/test/pleroma/web/rich_media/parser/ttl/opengraph_test.exs @@ -8,6 +8,7 @@ defmodule Pleroma.Web.RichMedia.Parser.TTL.OpengraphTest do import Mox + alias Pleroma.Tests.ObanHelpers alias Pleroma.UnstubbedConfigMock, as: ConfigMock alias Pleroma.Web.RichMedia.Card @@ -36,6 +37,18 @@ defmodule Pleroma.Web.RichMedia.Parser.TTL.OpengraphTest do Card.get_or_backfill_by_url(url) + # Find the backfill job + expected_job = + [ + worker: "Pleroma.Workers.RichMediaWorker", + args: %{"op" => "backfill", "url" => url} + ] + + assert_enqueued(expected_job) + + # Run it manually + ObanHelpers.perform_all() + assert_enqueued( worker: Pleroma.Workers.RichMediaWorker, args: %{"op" => "expire", "url" => url} From f00a681cc1dfd2f6b638eb8d7a03c5b82a5d5b90 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 19 Jun 2024 23:45:42 -0400 Subject: [PATCH 3/3] Change CI caching strategy Key the cache on the image being used and the commit sha. This should allow the cache to be reused by the same runner across multiple jobs/stages where appropriate. --- .gitlab-ci.yml | 4 +--- changelog.d/ci-cache.skip | 0 2 files changed, 1 insertion(+), 3 deletions(-) create mode 100644 changelog.d/ci-cache.skip diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index da2ddcf42..2fa519291 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -18,9 +18,7 @@ workflow: - if: $CI_COMMIT_BRANCH cache: &global_cache_policy - key: - files: - - mix.lock + key: $CI_JOB_IMAGE-$CI_COMMIT_SHORT_SHA paths: - deps - _build diff --git a/changelog.d/ci-cache.skip b/changelog.d/ci-cache.skip new file mode 100644 index 000000000..e69de29bb