From c3c14f5860051d0311ae34900ef0181ada5c630d Mon Sep 17 00:00:00 2001 From: Lain Soykaf Date: Tue, 21 Jan 2025 14:36:50 +0400 Subject: [PATCH 1/3] OAuth App: Fix test, optimize code. --- .../optimize-orphaned-apps-cleanup.change | 1 + lib/pleroma/web/o_auth/app.ex | 14 ++++--- test/pleroma/web/o_auth/app_test.exs | 42 ++++++++++++++++--- 3 files changed, 46 insertions(+), 11 deletions(-) create mode 100644 changelog.d/optimize-orphaned-apps-cleanup.change diff --git a/changelog.d/optimize-orphaned-apps-cleanup.change b/changelog.d/optimize-orphaned-apps-cleanup.change new file mode 100644 index 000000000..e86b75524 --- /dev/null +++ b/changelog.d/optimize-orphaned-apps-cleanup.change @@ -0,0 +1 @@ +- Optimize orphaned OAuth apps cleanup by using a single delete query instead of deleting records one by one. diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index 7661c2566..30a9d9134 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -168,16 +168,20 @@ defmodule Pleroma.Web.OAuth.App do @spec remove_orphans(pos_integer()) :: :ok def remove_orphans(limit \\ 100) do - fifteen_mins_ago = DateTime.add(DateTime.utc_now(), -900, :second) + fifteen_mins_ago = NaiveDateTime.add(NaiveDateTime.utc_now(), -900) - Repo.transaction(fn -> + # First get the IDs of apps to delete + app_ids = from(a in __MODULE__, where: is_nil(a.user_id) and a.inserted_at < ^fifteen_mins_ago, - limit: ^limit + limit: ^limit, + select: a.id ) |> Repo.all() - |> Enum.each(&Repo.delete(&1)) - end) + + # Then delete those specific apps + from(a in __MODULE__, where: a.id in ^app_ids) + |> Repo.delete_all() :ok end diff --git a/test/pleroma/web/o_auth/app_test.exs b/test/pleroma/web/o_auth/app_test.exs index 44219cf90..871b10ac3 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -55,19 +55,49 @@ defmodule Pleroma.Web.OAuth.AppTest do end test "removes orphaned apps" do + # Create an orphaned app (no user_id) attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} {:ok, %App{} = old_app} = App.get_or_make(attrs, ["write"]) - attrs = %{client_name: "PleromaFE", redirect_uris: "."} - {:ok, %App{} = app} = App.get_or_make(attrs, ["write"]) + # Create a non-orphaned app with a user + user = insert(:user) + attrs = %{client_name: "PleromaFE", redirect_uris: ".", user_id: user.id} + {:ok, %App{} = kept_app} = App.get_or_make(attrs, ["write"]) + + # Create an old but non-orphaned app + attrs = %{client_name: "OldButValid", redirect_uris: ".", user_id: user.id} + {:ok, %App{} = old_kept_app} = App.get_or_make(attrs, ["write"]) + + # backdate both old apps so they're within the threshold for being cleaned up + # 1 hour ago + past_time = NaiveDateTime.add(NaiveDateTime.utc_now(), -3600) - # backdate the old app so it's within the threshold for being cleaned up {:ok, _} = - "UPDATE apps SET inserted_at = now() - interval '1 hour' WHERE id = #{old_app.id}" - |> Pleroma.Repo.query() + Repo.query( + "UPDATE apps SET inserted_at = $1 WHERE id IN ($2, $3)", + [past_time, old_app.id, old_kept_app.id] + ) + + # Ensure the updates were applied + updated_app = Repo.get(App, old_app.id) + updated_kept_app = Repo.get(App, old_kept_app.id) + + assert NaiveDateTime.compare( + updated_app.inserted_at, + NaiveDateTime.add(NaiveDateTime.utc_now(), -900) + ) == :lt + + assert NaiveDateTime.compare( + updated_kept_app.inserted_at, + NaiveDateTime.add(NaiveDateTime.utc_now(), -900) + ) == :lt App.remove_orphans() - assert [app] == Pleroma.Repo.all(App) + # Verify the orphaned app was removed + assert is_nil(Repo.get(App, old_app.id)) + # Verify both non-orphaned apps still exist, regardless of age + assert Repo.get(App, kept_app.id) + assert Repo.get(App, old_kept_app.id) end end From d30d205f0b66acf8c67d25210cff5aab2b562b45 Mon Sep 17 00:00:00 2001 From: Lain Soykaf Date: Mon, 27 Jul 2026 11:57:16 +0400 Subject: [PATCH 2/3] fix(oauth): preserve dependent app cleanup --- .../optimize-orphaned-apps-cleanup.change | 2 +- lib/pleroma/web/o_auth/app.ex | 31 ++++++++++++------- test/pleroma/web/o_auth/app_test.exs | 12 ++++++- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/changelog.d/optimize-orphaned-apps-cleanup.change b/changelog.d/optimize-orphaned-apps-cleanup.change index e86b75524..3f037f125 100644 --- a/changelog.d/optimize-orphaned-apps-cleanup.change +++ b/changelog.d/optimize-orphaned-apps-cleanup.change @@ -1 +1 @@ -- Optimize orphaned OAuth apps cleanup by using a single delete query instead of deleting records one by one. +- Optimize orphaned OAuth apps cleanup by deleting records in batches instead of one by one. diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index 4ea02ee01..9a3dcfcea 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -8,7 +8,7 @@ defmodule Pleroma.Web.OAuth.App do import Ecto.Query alias Pleroma.Repo alias Pleroma.User - alias Pleroma.Web.OAuth.Token + alias Pleroma.Web.OAuth.{Authorization, Token} @type t :: %__MODULE__{} @@ -193,18 +193,25 @@ defmodule Pleroma.Web.OAuth.App do def remove_orphans(limit \\ 100) do fifteen_mins_ago = NaiveDateTime.add(NaiveDateTime.utc_now(), -900) - # First get the IDs of apps to delete - app_ids = - from(a in __MODULE__, - where: is_nil(a.user_id) and a.inserted_at < ^fifteen_mins_ago, - limit: ^limit, - select: a.id - ) - |> Repo.all() + Repo.transaction(fn -> + app_ids = + from(a in __MODULE__, + where: is_nil(a.user_id) and a.inserted_at < ^fifteen_mins_ago, + limit: ^limit, + lock: "FOR UPDATE SKIP LOCKED", + select: a.id + ) + |> Repo.all() - # Then delete those specific apps - from(a in __MODULE__, where: a.id in ^app_ids) - |> Repo.delete_all() + # Bulk deletes bypass the schema's association cleanup. + from(a in Authorization, where: a.app_id in ^app_ids) |> Repo.delete_all() + from(t in Token, where: t.app_id in ^app_ids) |> Repo.delete_all() + + from(a in __MODULE__, + where: a.id in ^app_ids and is_nil(a.user_id) and a.inserted_at < ^fifteen_mins_ago + ) + |> Repo.delete_all() + end) :ok end diff --git a/test/pleroma/web/o_auth/app_test.exs b/test/pleroma/web/o_auth/app_test.exs index 4d100cd13..26879d6ee 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -5,7 +5,9 @@ defmodule Pleroma.Web.OAuth.AppTest do use Pleroma.DataCase, async: true - alias Pleroma.Web.OAuth.App + alias Pleroma.MFA.Token, as: MFAToken + alias Pleroma.Web.OAuth.{App, Authorization, Token} + alias Pleroma.Web.Push.Subscription import Pleroma.Factory describe "get_or_make/2" do @@ -57,6 +59,10 @@ defmodule Pleroma.Web.OAuth.AppTest do test "removes orphaned apps" do attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} {:ok, %App{} = old_app} = App.get_or_make(attrs, ["write"]) + authorization = insert(:oauth_authorization, app: old_app) + token = insert(:oauth_token, app: old_app) + mfa_token = insert(:mfa_token, authorization: authorization) + push_subscription = insert(:push_subscription, token: token) user = insert(:user) attrs = %{client_name: "OldButValid", redirect_uris: ".", user_id: user.id} @@ -74,6 +80,10 @@ defmodule Pleroma.Web.OAuth.AppTest do App.remove_orphans() refute Repo.get(App, old_app.id) + refute Repo.get(Authorization, authorization.id) + refute Repo.get(Token, token.id) + refute Repo.get(MFAToken, mfa_token.id) + refute Repo.get(Subscription, push_subscription.id) assert Repo.get(App, recent_app.id) assert Repo.get(App, old_owned_app.id) end From 030c7e929c46210f408bc8a2c7f2fa603a9a80a5 Mon Sep 17 00:00:00 2001 From: Lain Soykaf Date: Mon, 27 Jul 2026 12:14:58 +0400 Subject: [PATCH 3/3] style(oauth): use single alias directives --- lib/pleroma/web/o_auth/app.ex | 3 ++- test/pleroma/web/o_auth/app_test.exs | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index 9a3dcfcea..9c16c23b6 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -8,7 +8,8 @@ defmodule Pleroma.Web.OAuth.App do import Ecto.Query alias Pleroma.Repo alias Pleroma.User - alias Pleroma.Web.OAuth.{Authorization, Token} + alias Pleroma.Web.OAuth.Authorization + alias Pleroma.Web.OAuth.Token @type t :: %__MODULE__{} diff --git a/test/pleroma/web/o_auth/app_test.exs b/test/pleroma/web/o_auth/app_test.exs index 26879d6ee..9a5045155 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -6,7 +6,9 @@ defmodule Pleroma.Web.OAuth.AppTest do use Pleroma.DataCase, async: true alias Pleroma.MFA.Token, as: MFAToken - alias Pleroma.Web.OAuth.{App, Authorization, Token} + alias Pleroma.Web.OAuth.App + alias Pleroma.Web.OAuth.Authorization + alias Pleroma.Web.OAuth.Token alias Pleroma.Web.Push.Subscription import Pleroma.Factory