Add WIP patches for better handling of received Deletes and Adds

patches/series

@@ -50,4 +50,10 @@ wip_20_federation-incoming-improve-link_resolve-retry-decis.patch
 wip_21_nodeinfo-lower-log-level-of-regular-actions-to-debug.patch
 wip_22_rich_media-lower-log-level-of-update.patch
 wip_23_Don-t-spam-logs-about-deleted-users.patch
-wip_24_dbg_log_receiver_crashes.patch
+wip_24_user-avoid-database-work-on-superfluous-pin.patch
+wip_25_Gracefully-ignore-Undo-activities-referring-to-unkno.patch
+wip_26_object_validators-only-query-relevant-table-for-obje.patch
+wip_27_transmogrifier-avoid-crashes-on-non-validation-Delte.patch
+wip_28_transmogrfier-be-more-selective-about-Delete-retry.patch
+wip_29_transmogrifier-gracefully-ignore-duplicated-object-d.patch
+wip_30_dbg_log_receiver_crashes.patch

patches/wip_24_dbg_log_receiver_crashes.patch

@@ -1,31 +0,0 @@
-From 5e5134d27d58801aeb33b360dbdb5048677caa4b Mon Sep 17 00:00:00 2001
-From: Oneric <oneric@oneric.stub>
-Date: Sun, 15 Dec 2024 02:50:20 +0100
-Subject: [PATCH] tmp/dbg: make sure crashes in receiver are actually logged
-MIME-Version: 1.0
-Content-Type: text/plain; charset=UTF-8
-Content-Transfer-Encoding: 8bit
-
-Rarely (once a day or so) there’s still a ReceiverWorker excception
-showing up in prometheus stats without any corresponding log message
----
- lib/pleroma/workers/receiver_worker.ex | 4 ++++
- 1 file changed, 4 insertions(+)
-
-diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex
-index cffdd8584..3c24c02ac 100644
---- a/lib/pleroma/workers/receiver_worker.ex
-+++ b/lib/pleroma/workers/receiver_worker.ex
-@@ -51,5 +51,9 @@ def perform(%Job{args: %{"op" => "incoming_ap_doc", "params" => params}}) do
-         Logger.error("Unexpected AP doc error: (raw) #{inspect(e)} from #{inspect(params)}")
-         {:error, e}
-     end
-+  rescue
-+    err ->
-+      Logger.error(Exception.format(:error, err, __STACKTRACE__))
-+      {:error, :crash}
-   end
- end
--- 
-2.39.5
-

patches/wip_24_user-avoid-database-work-on-superfluous-pin.patch

@@ -0,0 +1,34 @@
+From 8f0c462b1ba3963c5cd4c68bccef89339804b059 Mon Sep 17 00:00:00 2001
+From: Oneric <oneric@oneric.stub>
+Date: Tue, 17 Dec 2024 00:14:27 +0100
+Subject: [PATCH 1/7] user: avoid database work on superfluous pin
+
+The only thing this does is changing the updated_at field of the user.
+Afaict this came to be because prior to pins federating this was split
+into two functions, one of which created a changeset, the other applying
+a given changeset. When this was merged the bits were just copied into
+place.
+---
+ lib/pleroma/user.ex | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex
+index 2f4ac1b72..697597e1b 100644
+--- a/lib/pleroma/user.ex
++++ b/lib/pleroma/user.ex
+@@ -2581,10 +2581,10 @@ def add_pinned_object_id(%User{} = user, object_id) do
+           [pinned_objects: "You have already pinned the maximum number of statuses"]
+         end
+       end)
++      |> update_and_set_cache()
+     else
+-      change(user)
++      {:ok, user}
+     end
+-    |> update_and_set_cache()
+   end
+ 
+   @spec remove_pinned_object_id(User.t(), String.t()) :: {:ok, t()} | {:error, term()}
+-- 
+2.39.5
+

patches/wip_25_Gracefully-ignore-Undo-activities-referring-to-unkno.patch

@@ -0,0 +1,72 @@
+From 8c8a263f4b57064a3ff8b6bcf0979ff1ef7bd473 Mon Sep 17 00:00:00 2001
+From: Oneric <oneric@oneric.stub>
+Date: Wed, 18 Dec 2024 00:55:48 +0100
+Subject: [PATCH 2/7] Gracefully ignore Undo activities referring to unknown
+ objects
+
+It's quite common to receive spurious Deletes,
+so we neither want to waste resources on retrying
+nor spam "invalid AP" logs
+---
+ .../web/activity_pub/transmogrifier.ex        | 14 ++++++++++++++
+ .../web/activity_pub/transmogrifier_test.exs  | 19 +++++++++++++++++++
+ 2 files changed, 33 insertions(+)
+
+diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex
+index d8405bf75..0f34d0ae8 100644
+--- a/lib/pleroma/web/activity_pub/transmogrifier.ex
++++ b/lib/pleroma/web/activity_pub/transmogrifier.ex
+@@ -604,6 +604,20 @@ defp handle_incoming_normalised(
+        when type in ["Like", "EmojiReact", "Announce", "Block"] do
+     with {:ok, activity, _} <- Pipeline.common_pipeline(data, local: false) do
+       {:ok, activity}
++    else
++      {:error, {:validate, {:error, %Ecto.Changeset{errors: errors}}}} = e ->
++        # If we never saw the activity being undone, no need to do anything.
++        # Inspectinging the validation error content is a bit akward, but looking up the Activity
++        # ahead of time here would be too costly since Activity queries are not cached
++        # and there's no way atm to pass the retrieved result along along
++        if errors[:object] == {"can't find object", []} do
++          {:error, :ignore}
++        else
++          e
++        end
++
++      e ->
++        e
+     end
+   end
+ 
+diff --git a/test/pleroma/web/activity_pub/transmogrifier_test.exs b/test/pleroma/web/activity_pub/transmogrifier_test.exs
+index 94df25100..be07a0fe4 100644
+--- a/test/pleroma/web/activity_pub/transmogrifier_test.exs
++++ b/test/pleroma/web/activity_pub/transmogrifier_test.exs
+@@ -52,6 +52,25 @@ test "it works for incoming unfollows with an existing follow" do
+       refute User.following?(User.get_cached_by_ap_id(data["actor"]), user)
+     end
+ 
++    test "it ignores Undo activities for unknown objects" do
++      undo_data = %{
++        "id" => "https://remote.com/undo",
++        "type" => "Undo",
++        "actor" => "https:://remote.com/users/unknown",
++        "object" => %{
++          "id" => "https://remote.com/undone_activity/unknown",
++          "type" => "Like"
++        }
++      }
++
++      assert {:error, :ignore} == Transmogrifier.handle_incoming(undo_data)
++
++      user = insert(:user, local: false, ap_id: "https://remote.com/users/known")
++      undo_data = %{undo_data | "actor" => user.ap_id}
++
++      assert {:error, :ignore} == Transmogrifier.handle_incoming(undo_data)
++    end
++
+     test "it accepts Flag activities" do
+       user = insert(:user)
+       other_user = insert(:user)
+-- 
+2.39.5
+

patches/wip_26_object_validators-only-query-relevant-table-for-obje.patch

@@ -0,0 +1,94 @@
+From f3caccf7281b5ba57970d9a3c07cdddac98cd341 Mon Sep 17 00:00:00 2001
+From: Oneric <oneric@oneric.stub>
+Date: Wed, 18 Dec 2024 01:07:31 +0100
+Subject: [PATCH 3/7] object_validators: only query relevant table for object
+
+Most of them actually only accept either activities or a
+non-activity object later; querying both is then a waste
+of resources and may create false positives.
+---
+ .../activity_pub/object_validators/common_validations.ex    | 6 +++++-
+ .../web/activity_pub/object_validators/delete_validator.ex  | 5 ++++-
+ .../activity_pub/object_validators/emoji_react_validator.ex | 2 +-
+ .../web/activity_pub/object_validators/like_validator.ex    | 2 +-
+ .../web/activity_pub/object_validators/undo_validator.ex    | 2 +-
+ 5 files changed, 12 insertions(+), 5 deletions(-)
+
+diff --git a/lib/pleroma/web/activity_pub/object_validators/common_validations.ex b/lib/pleroma/web/activity_pub/object_validators/common_validations.ex
+index be5074348..f28cdca92 100644
+--- a/lib/pleroma/web/activity_pub/object_validators/common_validations.ex
++++ b/lib/pleroma/web/activity_pub/object_validators/common_validations.ex
+@@ -54,10 +54,14 @@ def validate_actor_presence(cng, options \\ []) do
+   def validate_object_presence(cng, options \\ []) do
+     field_name = Keyword.get(options, :field_name, :object)
+     allowed_types = Keyword.get(options, :allowed_types, false)
++    allowed_categories = Keyword.get(options, :allowed_object_categores, [:object, :activity])
+ 
+     cng
+     |> validate_change(field_name, fn field_name, object_id ->
+-      object = Object.get_cached_by_ap_id(object_id) || Activity.get_by_ap_id(object_id)
++      object =
++        (:object in allowed_categories && Object.get_cached_by_ap_id(object_id)) ||
++          (:activity in allowed_categories && Activity.get_by_ap_id(object_id)) ||
++          nil
+ 
+       cond do
+         !object ->
+diff --git a/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex b/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex
+index a08e8ebe0..2dcb9a5d6 100644
+--- a/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex
++++ b/lib/pleroma/web/activity_pub/object_validators/delete_validator.ex
+@@ -61,7 +61,10 @@ defp validate_data(cng) do
+     |> validate_inclusion(:type, ["Delete"])
+     |> validate_delete_actor(:actor)
+     |> validate_modification_rights()
+-    |> validate_object_or_user_presence(allowed_types: @deletable_types)
++    |> validate_object_or_user_presence(
++      allowed_types: @deletable_types,
++      allowed_object_categories: [:object]
++    )
+     |> add_deleted_activity_id()
+   end
+ 
+diff --git a/lib/pleroma/web/activity_pub/object_validators/emoji_react_validator.ex b/lib/pleroma/web/activity_pub/object_validators/emoji_react_validator.ex
+index bda67feee..9cafeeb14 100644
+--- a/lib/pleroma/web/activity_pub/object_validators/emoji_react_validator.ex
++++ b/lib/pleroma/web/activity_pub/object_validators/emoji_react_validator.ex
+@@ -129,7 +129,7 @@ defp validate_data(data_cng) do
+     |> validate_inclusion(:type, ["EmojiReact"])
+     |> validate_required([:id, :type, :object, :actor, :context, :to, :cc, :content])
+     |> validate_actor_presence()
+-    |> validate_object_presence()
++    |> validate_object_presence(allowed_object_categories: [:object])
+     |> validate_emoji()
+     |> maybe_validate_tag_presence()
+   end
+diff --git a/lib/pleroma/web/activity_pub/object_validators/like_validator.ex b/lib/pleroma/web/activity_pub/object_validators/like_validator.ex
+index 35e000d72..44bb0c238 100644
+--- a/lib/pleroma/web/activity_pub/object_validators/like_validator.ex
++++ b/lib/pleroma/web/activity_pub/object_validators/like_validator.ex
+@@ -66,7 +66,7 @@ defp validate_data(data_cng) do
+     |> validate_inclusion(:type, ["Like"])
+     |> validate_required([:id, :type, :object, :actor, :context, :to, :cc])
+     |> validate_actor_presence()
+-    |> validate_object_presence()
++    |> validate_object_presence(allowed_object_categories: [:object])
+     |> validate_existing_like()
+   end
+ 
+diff --git a/lib/pleroma/web/activity_pub/object_validators/undo_validator.ex b/lib/pleroma/web/activity_pub/object_validators/undo_validator.ex
+index 703643e3f..06516f6c7 100644
+--- a/lib/pleroma/web/activity_pub/object_validators/undo_validator.ex
++++ b/lib/pleroma/web/activity_pub/object_validators/undo_validator.ex
+@@ -44,7 +44,7 @@ defp validate_data(data_cng) do
+     |> validate_inclusion(:type, ["Undo"])
+     |> validate_required([:id, :type, :object, :actor, :to, :cc])
+     |> validate_undo_actor(:actor)
+-    |> validate_object_presence()
++    |> validate_object_presence(allowed_object_categories: [:activity])
+     |> validate_undo_rights()
+   end
+ 
+-- 
+2.39.5
+

patches/wip_27_transmogrifier-avoid-crashes-on-non-validation-Delte.patch

@@ -0,0 +1,33 @@
+From 468c4e3cd34aa2f997fac46592667e9ff3b8bf1d Mon Sep 17 00:00:00 2001
+From: Oneric <oneric@oneric.stub>
+Date: Wed, 18 Dec 2024 01:21:56 +0100
+Subject: [PATCH 4/7] transmogrifier: avoid crashes on non-validation Delte
+ errors
+
+Happens e.g. for duplicated Deletes.
+The remaining tombstone object no longer has an actor,
+leading to an error response during side-effect handling.
+---
+ lib/pleroma/web/activity_pub/transmogrifier.ex | 6 ++++++
+ 1 file changed, 6 insertions(+)
+
+diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex
+index 0f34d0ae8..bb6e2cb0d 100644
+--- a/lib/pleroma/web/activity_pub/transmogrifier.ex
++++ b/lib/pleroma/web/activity_pub/transmogrifier.ex
+@@ -572,6 +572,12 @@ defp handle_incoming_normalised(
+         else
+           _ -> e
+         end
++
++      {:error, _} = e ->
++        e
++
++      e ->
++        {:error, e}
+     end
+   end
+ 
+-- 
+2.39.5
+

patches/wip_28_transmogrfier-be-more-selective-about-Delete-retry.patch

@@ -0,0 +1,55 @@
+From 8f3fb422e02a1795483fee9e9ecbcd2d8c7145aa Mon Sep 17 00:00:00 2001
+From: Oneric <oneric@oneric.stub>
+Date: Wed, 18 Dec 2024 01:27:32 +0100
+Subject: [PATCH 5/7] transmogrfier: be more selective about Delete retry
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+If something else renders the Delete invalid,
+there’s no point in retrying anyway
+---
+ .../web/activity_pub/transmogrifier.ex        | 26 ++++++++++++-------
+ 1 file changed, 16 insertions(+), 10 deletions(-)
+
+diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex
+index bb6e2cb0d..7954d979c 100644
+--- a/lib/pleroma/web/activity_pub/transmogrifier.ex
++++ b/lib/pleroma/web/activity_pub/transmogrifier.ex
+@@ -560,17 +560,23 @@ defp handle_incoming_normalised(
+            Pipeline.common_pipeline(data, local: false) do
+       {:ok, activity}
+     else
+-      {:error, {:validate, _}} = e ->
+-        # Check if we have a create activity for this
+-        with {:ok, object_id} <- ObjectValidators.ObjectID.cast(data["object"]),
+-             %Activity{data: %{"actor" => actor}} <-
+-               Activity.create_by_object_ap_id(object_id) |> Repo.one(),
+-             # We have one, insert a tombstone and retry
+-             {:ok, tombstone_data, _} <- Builder.tombstone(actor, object_id),
+-             {:ok, _tombstone} <- Object.create(tombstone_data) do
+-          handle_incoming(data)
++      {:error, {:validate, {:error, %Ecto.Changeset{errors: errors}}}} = e ->
++        if errors[:object] == {"can't find object", []} do
++          # Check if we have a create activity for this
++          # (e.g. from a db prune without --prune-activities)
++          # We'd still like to process side effects so insert a tombstone and retry
++          with {:ok, object_id} <- ObjectValidators.ObjectID.cast(data["object"]),
++               %Activity{data: %{"actor" => actor}} <-
++                 Activity.create_by_object_ap_id(object_id) |> Repo.one(),
++               # We have one, insert a tombstone and retry
++               {:ok, tombstone_data, _} <- Builder.tombstone(actor, object_id),
++               {:ok, _tombstone} <- Object.create(tombstone_data) do
++            handle_incoming(data)
++          else
++            _ -> e
++          end
+         else
+-          _ -> e
++          e
+         end
+ 
+       {:error, _} = e ->
+-- 
+2.39.5
+

patches/wip_29_transmogrifier-gracefully-ignore-duplicated-object-d.patch

@@ -0,0 +1,76 @@
+From fa3f0457a777a5a49a2217e07357c6e7cabd16fb Mon Sep 17 00:00:00 2001
+From: Oneric <oneric@oneric.stub>
+Date: Wed, 18 Dec 2024 01:34:33 +0100
+Subject: [PATCH 6/7] transmogrifier: gracefully ignore duplicated object
+ deletes
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+The object lookup is later repeated in the validator, but due to
+caching shouldn't incur any noticeable performance impact.
+It’s actually preferable to check here, since it avoids the otherwise
+occuring user lookup and overhead from starting and aborting a
+transaction
+---
+ lib/pleroma/object.ex                          |  5 +++++
+ lib/pleroma/web/activity_pub/transmogrifier.ex | 18 ++++++++++++++----
+ 2 files changed, 19 insertions(+), 4 deletions(-)
+
+diff --git a/lib/pleroma/object.ex b/lib/pleroma/object.ex
+index 379b361f8..f5797867c 100644
+--- a/lib/pleroma/object.ex
++++ b/lib/pleroma/object.ex
+@@ -216,6 +216,11 @@ def get_cached_by_ap_id(ap_id) do
+     end
+   end
+ 
++  # Intentionally accepts non-Object arguments!
++  @spec is_tombstone_object?(term()) :: boolean()
++  def is_tombstone_object?(%Object{data: %{"type" => "Tombstone"}}), do: true
++  def is_tombstone_object?(_), do: false
++
+   def make_tombstone(%Object{data: %{"id" => id, "type" => type}}, deleted \\ DateTime.utc_now()) do
+     %ObjectTombstone{
+       id: id,
+diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex
+index 7954d979c..b890f573d 100644
+--- a/lib/pleroma/web/activity_pub/transmogrifier.ex
++++ b/lib/pleroma/web/activity_pub/transmogrifier.ex
+@@ -556,19 +556,29 @@ defp handle_incoming_normalised(
+          %{"type" => "Delete"} = data,
+          _options
+        ) do
+-    with {:ok, activity, _} <-
+-           Pipeline.common_pipeline(data, local: false) do
++    oid_result = ObjectValidators.ObjectID.cast(data["object"])
++
++    with {_, {:ok, object_id}} <- {:object_id, oid_result},
++         object <- Object.get_cached_by_ap_id(object_id),
++         {_, false} <- {:tombstone, Object.is_tombstone_object?(object) && !data["actor"]},
++         {:ok, activity, _} <- Pipeline.common_pipeline(data, local: false) do
+       {:ok, activity}
+     else
++      {:object_id, _} ->
++        {:error, {:validate, "Invalid object id: #{data["object"]}"}}
++
++      {:tombstone, true} ->
++        {:error, :ignore}
++
+       {:error, {:validate, {:error, %Ecto.Changeset{errors: errors}}}} = e ->
+         if errors[:object] == {"can't find object", []} do
+           # Check if we have a create activity for this
+           # (e.g. from a db prune without --prune-activities)
+-          # We'd still like to process side effects so insert a tombstone and retry
++          # We'd still like to process side effects so insert a fake tombstone and retry
++          # (real tombstones from Object.delete do not have an actor field)
+           with {:ok, object_id} <- ObjectValidators.ObjectID.cast(data["object"]),
+                %Activity{data: %{"actor" => actor}} <-
+                  Activity.create_by_object_ap_id(object_id) |> Repo.one(),
+-               # We have one, insert a tombstone and retry
+                {:ok, tombstone_data, _} <- Builder.tombstone(actor, object_id),
+                {:ok, _tombstone} <- Object.create(tombstone_data) do
+             handle_incoming(data)
+-- 
+2.39.5
+

patches/wip_30_dbg_log_receiver_crashes.patch

@@ -0,0 +1,39 @@
+From 2af623e81b3a84ff658ff3dba4f534f59468f563 Mon Sep 17 00:00:00 2001
+From: Oneric <oneric@oneric.stub>
+Date: Sun, 15 Dec 2024 02:50:20 +0100
+Subject: [PATCH 7/7] tmp/dbg: make sure crashes in receiver are actually
+ logged
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+Rarely (once a day or so) there’s still a ReceiverWorker excception
+showing up in prometheus stats without any corresponding log message
+---
+ lib/pleroma/workers/receiver_worker.ex | 9 ++++++++-
+ 1 file changed, 8 insertions(+), 1 deletion(-)
+
+diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex
+index cffdd8584..61f3ce7f1 100644
+--- a/lib/pleroma/workers/receiver_worker.ex
++++ b/lib/pleroma/workers/receiver_worker.ex
+@@ -48,8 +48,15 @@ def perform(%Job{args: %{"op" => "incoming_ap_doc", "params" => params}}) do
+         e
+ 
+       e ->
+-        Logger.error("Unexpected AP doc error: (raw) #{inspect(e)} from #{inspect(params)}")
++        Logger.error("Unexpected AP doc error: #{inspect(e)} from #{inspect(params)}")
+         {:error, e}
+     end
++  rescue
++    err ->
++      Logger.error(
++        "Receiver worker CRASH on #{params} with: #{Exception.format(:error, err, __STACKTRACE__)}"
++      )
++
++      {:error, :crash}
+   end
+ end
+-- 
+2.39.5
+