patches/wip: improve link_resolv retry decision and unify validation error tuple

This commit is contained in:
Oneric 2024-12-14 20:29:46 +01:00
parent 507426d164
commit aa01097960
8 changed files with 161 additions and 46 deletions

View file

@ -46,5 +46,6 @@ wip_16_stats-use-cheaper-peers-query.patch
wip_17_stats-estimate-remote-user-count.patch
wip_18_federator-don-t-nest-error-_-tuples.patch
wip_19_Error-out-earlier-on-missing-mandatory-reference.patch
wip_20_nodeinfo-lower-log-level-of-regular-actions-to-debug.patch
wip_21_rich_media-lower-log-level-of-update.patch
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

View file

@ -1,17 +1,17 @@
From b112466c12bae465a90b4941be6d5cdcea525a56 Mon Sep 17 00:00:00 2001
From 6830693d6877345d6cbcb576bdacb233c9c37ad3 Mon Sep 17 00:00:00 2001
From: Oneric <oneric@oneric.stub>
Date: Sat, 14 Dec 2024 02:02:04 +0000
Subject: [PATCH 05/22] validators/add_remove: don't crash on failure to
resolve reference
Subject: [PATCH] validators/add_remove: don't crash on failure to resolve
reference
It allows for informed error handling and retry/discard job
decisions lateron which a future commit will add.
---
.../object_validators/add_remove_validator.ex | 23 ++++++++++++-------
1 file changed, 15 insertions(+), 8 deletions(-)
.../object_validators/add_remove_validator.ex | 24 ++++++++++++-------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/lib/pleroma/web/activity_pub/object_validators/add_remove_validator.ex b/lib/pleroma/web/activity_pub/object_validators/add_remove_validator.ex
index a960e99b1..44589b890 100644
index a960e99b1..e05ade53f 100644
--- a/lib/pleroma/web/activity_pub/object_validators/add_remove_validator.ex
+++ b/lib/pleroma/web/activity_pub/object_validators/add_remove_validator.ex
@@ -9,6 +9,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.AddRemoveValidator do
@ -22,7 +22,7 @@ index a960e99b1..44589b890 100644
alias Pleroma.User
@@ -27,14 +28,20 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.AddRemoveValidator do
@@ -27,14 +28,21 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.AddRemoveValidator do
end
def cast_and_validate(data) do
@ -42,8 +42,9 @@ index a960e99b1..44589b890 100644
+ |> validate_data(actor)
+ else
+ {:feataddr, _} ->
+ Logger.info("Cannot verify featured collection address for #{data["id"]}")
+ {:error, :invalid}
+ {:error,
+ {:validate,
+ "Actor doesn't provide featured collection address to verify against: #{data["id"]}"}}
+
+ {:user, _} ->
+ {:error, :link_resolve_failed}

View file

@ -1,7 +1,7 @@
From 7bf8bcc3c21bd6a155b5627c94b25dbc73f036f1 Mon Sep 17 00:00:00 2001
From 61429541bce63902e0dfe406bfaedb23e87295bf Mon Sep 17 00:00:00 2001
From: Oneric <oneric@oneric.stub>
Date: Sat, 23 Nov 2024 22:50:20 +0100
Subject: [PATCH 13/22] receiver_worker: don't reattempt invalid documents
Subject: [PATCH] receiver_worker: don't reattempt invalid documents
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
@ -19,7 +19,7 @@ seem kinda frequent, don't log at error, only info level.
1 file changed, 12 insertions(+)
diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex
index b28442042..0caab41c0 100644
index b28442042..193b500f4 100644
--- a/lib/pleroma/workers/receiver_worker.ex
+++ b/lib/pleroma/workers/receiver_worker.ex
@@ -3,6 +3,8 @@
@ -36,13 +36,13 @@ index b28442042..0caab41c0 100644
{:discard, :already_present}
+ # invalid data or e.g. deleting an object we don't know about anyway
+ {:error, {:error, {:validate, ecto_changeset}}} ->
+ Logger.info("Received invalid AP document: #{inspect(ecto_changeset)}")
+ {:error, {:error, {:validate, issue}}} ->
+ Logger.info("Received invalid AP document: #{inspect(issue)}")
+ {:discard, :invalid}
+
+ # rarer, but sometimes theres an additional :error in front
+ {:error, {:error, {:error, {:validate, ecto_changeset}}}} ->
+ Logger.info("Received invalid AP document: (3e) #{inspect(ecto_changeset)}")
+ {:error, {:error, {:error, {:validate, issue}}}} ->
+ Logger.info("Received invalid AP document: (3e) #{inspect(issue)}")
+ {:discard, :invalid}
+
{:error, _} = e ->

View file

@ -1,26 +1,27 @@
From 5285aa86dd5c30514fe4c857bae0e6b3ee3ae68c Mon Sep 17 00:00:00 2001
From ae12775b3e9545cdd67adeb6933eccc4b5aaadd3 Mon Sep 17 00:00:00 2001
From: Oneric <oneric@oneric.stub>
Date: Sat, 14 Dec 2024 15:40:52 +0100
Subject: [PATCH 1/4] federator: don't nest {:error, _} tuples
Subject: [PATCH] federator: don't nest {:error, _} tuples
It makes decisions based on error sources harder since all possible
nesting levels need to be checked for. As shown by the return values
handled in the receiver worker something else still nests those,
but this is a first start.
---
lib/pleroma/web/federator.ex | 5 ++++-
lib/pleroma/web/federator.ex | 6 +++++-
lib/pleroma/workers/receiver_worker.ex | 4 ++--
2 files changed, 6 insertions(+), 3 deletions(-)
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/lib/pleroma/web/federator.ex b/lib/pleroma/web/federator.ex
index adf8298da..e5a2f9efe 100644
index adf8298da..ce1dd8fa8 100644
--- a/lib/pleroma/web/federator.ex
+++ b/lib/pleroma/web/federator.ex
@@ -117,7 +117,10 @@ def perform(:incoming_ap_doc, params) do
@@ -117,7 +117,11 @@ def perform(:incoming_ap_doc, params) do
e ->
# Just drop those for now
Logger.debug(fn -> "Unhandled activity\n" <> Jason.encode!(params, pretty: true) end)
- {:error, e}
+
+ case e do
+ {:error, _} -> e
+ _ -> {:error, e}
@ -29,24 +30,24 @@ index adf8298da..e5a2f9efe 100644
end
end
diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex
index 00cd47501..f82d1b8cc 100644
index 6a8f89a03..e6e2a82e2 100644
--- a/lib/pleroma/workers/receiver_worker.ex
+++ b/lib/pleroma/workers/receiver_worker.ex
@@ -24,12 +24,12 @@ def perform(%Job{args: %{"op" => "incoming_ap_doc", "params" => params}}) do
{:discard, :already_present}
# invalid data or e.g. deleting an object we don't know about anyway
- {:error, {:error, {:validate, ecto_changeset}}} ->
+ {:error, {:validate, ecto_changeset}} ->
Logger.info("Received invalid AP document: #{inspect(ecto_changeset)}")
- {:error, {:error, {:validate, issue}}} ->
+ {:error, {:validate, issue}} ->
Logger.info("Received invalid AP document: #{inspect(issue)}")
{:discard, :invalid}
# rarer, but sometimes theres an additional :error in front
- {:error, {:error, {:error, {:validate, ecto_changeset}}}} ->
+ {:error, {:error, {:validate, ecto_changeset}}} ->
Logger.info("Received invalid AP document: (3e) #{inspect(ecto_changeset)}")
- {:error, {:error, {:error, {:validate, issue}}}} ->
+ {:error, {:error, {:validate, issue}}} ->
Logger.info("Received invalid AP document: (3e) #{inspect(issue)}")
{:discard, :invalid}
--
2.47.1
2.39.5

View file

@ -1,10 +1,7 @@
From b3ac2ebdad979f8b7436384b3ae5b00eb7ece4f1 Mon Sep 17 00:00:00 2001
From 31d85682864a1d12905cb1d848274168b31121fc Mon Sep 17 00:00:00 2001
From: Oneric <oneric@oneric.stub>
Date: Wed, 4 Dec 2024 02:09:49 +0100
Subject: [PATCH 2/4] Error out earlier on missing mandatory reference
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Subject: [PATCH] Error out earlier on missing mandatory reference
This is the only user of fetch_actor_and_object which previously just
always preteneded to be successful. For all the activity types handled
@ -14,14 +11,14 @@ unknown remote objects is desirable in the first place is up for debate)
All other users of the similar fetch_actor already properly check success.
TODO: if the return value is 404, use {:error, :ignore} instead,
the reference is most likely a private object were not allowed to access
or already deleted
Note, this currently lumps all reolv failure reasons together,
so even e.g. boosts of MRF rejected posts will still exhaust all
retries. The following commit improves on this.
---
lib/pleroma/web/activity_pub/object_validator.ex | 9 ++++++---
lib/pleroma/web/activity_pub/transmogrifier.ex | 5 ++++-
lib/pleroma/workers/receiver_worker.ex | 5 +++++
3 files changed, 15 insertions(+), 4 deletions(-)
lib/pleroma/workers/receiver_worker.ex | 7 +++++++
3 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/lib/pleroma/web/activity_pub/object_validator.ex b/lib/pleroma/web/activity_pub/object_validator.ex
index cb0cc9ed7..e44f2fdee 100644
@ -63,21 +60,23 @@ index 18015e07e..e8d112727 100644
{:error, e}
end
diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex
index f82d1b8cc..aaaae636e 100644
index e6e2a82e2..d7a2a9c86 100644
--- a/lib/pleroma/workers/receiver_worker.ex
+++ b/lib/pleroma/workers/receiver_worker.ex
@@ -33,6 +33,11 @@ def perform(%Job{args: %{"op" => "incoming_ap_doc", "params" => params}}) do
Logger.info("Received invalid AP document: (3e) #{inspect(ecto_changeset)}")
@@ -33,6 +33,13 @@ def perform(%Job{args: %{"op" => "incoming_ap_doc", "params" => params}}) do
Logger.info("Received invalid AP document: (3e) #{inspect(issue)}")
{:discard, :invalid}
+ # failed to resolve a necessary referenced remote AP object;
+ # might be temporary server/network trouble thus reattempt
+ {:error, :link_resolve_failed} = e ->
+ # TODO: lower to debug for PR!
+ Logger.info("Failed to resolve AP link; may retry: #{inspect(params)}")
+ e
+
{:error, _} = e ->
Logger.error("Unexpected AP doc error: #{inspect(e)} from #{inspect(params)}")
e
--
2.47.1
2.39.5

View file

@ -0,0 +1,113 @@
From f3d75031e8cb6ba321016605b36ecf5d949c547d Mon Sep 17 00:00:00 2001
From: Oneric <oneric@oneric.stub>
Date: Sat, 14 Dec 2024 19:33:42 +0100
Subject: [PATCH] federation/incoming: improve link_resolve retry decision
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
To facilitate this ObjectValidator.fetch_actor_and_object is adapted to
return an informative error. Otherwise wed be unable to make an
informed decision on retrying or not later. Theres no point in
retrying to fetch MRF-blocked stuff or private posts for example.
---
lib/pleroma/user.ex | 4 ++++
.../web/activity_pub/object_validator.ex | 21 +++++++++++++++++--
.../web/activity_pub/transmogrifier.ex | 9 ++++++++
lib/pleroma/workers/receiver_worker.ex | 3 +++
4 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex
index 3042d86f2..2e09a89b6 100644
--- a/lib/pleroma/user.ex
+++ b/lib/pleroma/user.ex
@@ -1999,6 +1999,10 @@ def get_or_fetch_by_ap_id(ap_id, options \\ []) do
{%User{} = user, _} ->
{:ok, user}
+ {_, {:error, {:reject, :mrf}}} ->
+ Logger.debug("Rejected to fetch user due to MRF: #{ap_id}")
+ {:error, {:reject, :mrf}}
+
e ->
Logger.error("Could not fetch user #{ap_id}, #{inspect(e)}")
{:error, :not_found}
diff --git a/lib/pleroma/web/activity_pub/object_validator.ex b/lib/pleroma/web/activity_pub/object_validator.ex
index e44f2fdee..93980f35b 100644
--- a/lib/pleroma/web/activity_pub/object_validator.ex
+++ b/lib/pleroma/web/activity_pub/object_validator.ex
@@ -15,6 +15,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidator do
alias Pleroma.EctoType.ActivityPub.ObjectValidators
alias Pleroma.Object
alias Pleroma.Object.Containment
+ alias Pleroma.Object.Fetcher
alias Pleroma.User
alias Pleroma.Web.ActivityPub.ObjectValidators.AcceptRejectValidator
alias Pleroma.Web.ActivityPub.ObjectValidators.AddRemoveValidator
@@ -253,11 +254,27 @@ def fetch_actor(object) do
end
def fetch_actor_and_object(object) do
+ # Fetcher.fetch_object_from_id already first does a local db lookup
with {:ok, %User{}} <- fetch_actor(object),
- %Object{} <- Object.normalize(object["object"], fetch: true) do
+ {:ap_id, id} when is_binary(id) <-
+ {:ap_id, Pleroma.Web.ActivityPub.Utils.get_ap_id(object["object"])},
+ {:ok, %Object{}} <- Fetcher.fetch_object_from_id(id) do
:ok
else
- _ -> :error
+ {:ap_id, id} ->
+ {:error, {:validate, "Invalid AP id: #{inspect(id)}"}}
+
+ # if actor: late post from a previously unknown, deleted profile
+ # if object: private post we're not allowed to access
+ # (other HTTP replies might just indicate a temporary network failure though!)
+ {:error, e} when e in [:not_found, :forbidden] ->
+ {:error, :ignore}
+
+ {:error, _} = e ->
+ e
+
+ e ->
+ {:error, e}
end
end
diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex
index e8d112727..d8405bf75 100644
--- a/lib/pleroma/web/activity_pub/transmogrifier.ex
+++ b/lib/pleroma/web/activity_pub/transmogrifier.ex
@@ -523,6 +523,15 @@ defp handle_incoming_normalised(%{"type" => type} = data, _options)
{:ok, activity, _meta} <- Pipeline.common_pipeline(data, local: false) do
{:ok, activity}
else
+ {:link, {:error, :ignore}} ->
+ {:error, :ignore}
+
+ {:link, {:error, {:validate, _}} = e} ->
+ e
+
+ {:link, {:error, {:reject, _}} = e} ->
+ e
+
{:link, _} ->
{:error, :link_resolve_failed}
diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex
index f79a0da42..0794e84c9 100644
--- a/lib/pleroma/workers/receiver_worker.ex
+++ b/lib/pleroma/workers/receiver_worker.ex
@@ -23,6 +23,9 @@ def perform(%Job{args: %{"op" => "incoming_ap_doc", "params" => params}}) do
{:error, :already_present} ->
{:discard, :already_present}
+ {:error, :ignore} ->
+ {:discard, :ignore}
+
# invalid data or e.g. deleting an object we don't know about anyway
{:error, {:validate, issue}} ->
Logger.info("Received invalid AP document: #{inspect(issue)}")
--
2.39.5