AkkomaGang/akkoma
AkkomaGang/akkoma
15
66
Fork 94
Code Issues 180 Pull requests 27 Projects 2 Releases 14 Packages Wiki Activity

Handle failed fetches a bit better #743

Merged
floatingghost merged 34 commits from failed-fetch-processing into develop 2024-04-19 11:25:14 +00:00
Conversation 7 Commits 34 Files changed 18 +365 -85
floatingghost commented 2024-04-13 22:56:27 +00:00
Owner
Copy link

pulls most of https://git.pleroma.social/pleroma/pleroma/-/merge_requests/4015 and adapts it with the various little changes we've made to this stuff

pulls most of https://git.pleroma.social/pleroma/pleroma/-/merge_requests/4015 and adapts it with the various little changes we've made to this stuff
floatingghost added 26 commits 2024-04-13 22:56:27 +00:00 
It was only being called once and can be replaced with a case statement.
These tests relied on the removed Fetcher.fetch_object_from_id!/2 function injecting the error tuple into a log message with the exact words "Object containment failed."

We will keep this behavior by generating a similar log message, but perhaps this should do a better job of matching on the error tuple returned by Transmogrifier.handle_incoming/1
This is a definite sign the instance is blocked and they are enforcing authorized_fetch
This reverts commit d472bafec19cee269e7c943bafae7c805785acd7.
We were overzealous with matching on a raw error from the object fetch that should have never been relied on like this. If we can't fetch successfully we should assume that the collection is private.

Building a more expressive and universal error struct to match on may be something to consider.
Object fetch errors are logged in the fetcher module
fix pattern matching in fetch errors
Some checks failed
ci/woodpecker/push/build-amd64 Pipeline is pending
Details
ci/woodpecker/push/build-arm64 Pipeline is pending
Details
ci/woodpecker/push/docs Pipeline is pending
Details
ci/woodpecker/push/lint Pipeline is pending
Details
ci/woodpecker/push/test Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/test Pipeline was successful
Details
ci/woodpecker/pr/build-arm64 unknown status
Details
ci/woodpecker/pr/build-amd64 unknown status
Details
ci/woodpecker/pr/docs unknown status
Details
2fc25980d1
floatingghost commented 2024-04-13 23:22:44 +00:00
Author
Owner
Copy link

gonna document issues as i see them

user fetch validation can cause oban :error, so can ID collisions

{:transmogrifier, {:error, {:validate, ... }}}
Apr 14 00:19:57 mix[1271365]: - `:ok`
Apr 14 00:19:57 mix[1271365]: - `:discard`
Apr 14 00:19:57 mix[1271365]: - `{:ok, value}`
Apr 14 00:19:57 mix[1271365]: - `{:error, reason}`,
Apr 14 00:19:57 mix[1271365]: - `{:cancel, reason}`
Apr 14 00:19:57 mix[1271365]: - `{:discard, reason}`
Apr 14 00:19:57 mix[1271365]: - `{:snooze, seconds}`
Apr 14 00:19:57 mix[1271365]: Instead received:
Apr 14 00:19:57 mix[1271365]: :error
gonna document issues as i see them user fetch validation can cause oban :error, so can ID collisions ``` {:transmogrifier, {:error, {:validate, ... }}} Apr 14 00:19:57 mix[1271365]: - `:ok` Apr 14 00:19:57 mix[1271365]: - `:discard` Apr 14 00:19:57 mix[1271365]: - `{:ok, value}` Apr 14 00:19:57 mix[1271365]: - `{:error, reason}`, Apr 14 00:19:57 mix[1271365]: - `{:cancel, reason}` Apr 14 00:19:57 mix[1271365]: - `{:discard, reason}` Apr 14 00:19:57 mix[1271365]: - `{:snooze, seconds}` Apr 14 00:19:57 mix[1271365]: Instead received: Apr 14 00:19:57 mix[1271365]: :error ```
floatingghost commented 2024-04-13 23:24:51 +00:00
Author
Owner
Copy link

ah right it probably doesn't hit the pattern match in remotefetcher

ah right it probably doesn't hit the pattern match in remotefetcher
Oneric commented 2024-04-14 01:08:13 +00:00
Member
Copy link

ah right it probably doesn't hit the pattern match in remotefetcher

yep, Oban wants {:error, _info} but for everyting not explicitly matched, the last catch all just returns :error:, that oversight was fixed up on Pleroma’s side with https://git.pleroma.social/pleroma/pleroma/-/merge_requests/4077

> ah right it probably doesn't hit the pattern match in remotefetcher yep, Oban wants `{:error, _info}` but for everyting not explicitly matched, the last catch all just returns `:error:`, that oversight was fixed up on Pleroma’s side with https://git.pleroma.social/pleroma/pleroma/-/merge_requests/4077
Oneric commented 2024-04-14 01:34:56 +00:00
Member
Copy link

Also a question: the ported changes make an effort to sort the job into :discard instead of :error to avoid retries, but afaict all remote_fetcher jobs currently (by default) only have a single attempt anyway, so no retries should occur in the first place?
This is based on RemoteFetcherWorker using WorkerHelper, which sets the queue’s default max_attempts to 1 and overrides this for enqueued jobs based on a config value if present, but iinm there’s no default config value for remote_fetcher.

Oban docs say by default job uniqueness is checked across all states except :discarded and :cancelled. If jobs were retryable and we now discard jobs rather than exhausting all attempts with a backoff, won’t this in theory allow bad jobs to be reattempted via insert faster than it previously did with a backoff?

(the changes are still good to have, but just checking i didn’t misunderstand something here)

Also a question: the ported changes make an effort to sort the job into `:discard` instead of `:error` to avoid retries, but afaict all `remote_fetcher` jobs currently (by default) only have a single attempt anyway, so no retries should occur in the first place? This is based on `RemoteFetcherWorker` using `WorkerHelper`, which sets the queue’s default `max_attempts` to `1` and overrides this for enqueued jobs based on a config value if present, but iinm there’s no default config value for `remote_fetcher`. [Oban docs](https://hexdocs.pm/oban/2.15.2/Oban.html#module-unique-jobs) say by default job uniqueness is checked across all states except `:discarded` and `:cancelled`. If jobs were retryable and we now discard jobs rather than exhausting all attempts with a backoff, won’t this in theory allow bad jobs to be reattempted via insert _faster_ than it previously did with a backoff? *(the changes are still good to have, but just checking i didn’t misunderstand something here)*
floatingghost added 2 commits 2024-04-16 01:36:10 +00:00
changelog entry
Some checks are pending
ci/woodpecker/push/build-amd64 Pipeline is pending
Details
ci/woodpecker/push/test Pipeline is pending
Details
ci/woodpecker/pr/build-amd64 Pipeline is pending
Details
ci/woodpecker/pr/build-arm64 Pipeline is pending
Details
ci/woodpecker/push/build-arm64 Pipeline is pending
Details
ci/woodpecker/push/docs Pipeline is pending
Details
ci/woodpecker/push/lint Pipeline is pending
Details
ci/woodpecker/pr/docs Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline is pending
Details
ci/woodpecker/pr/test Pipeline is pending
Details
1896ff1ab0
floatingghost commented 2024-04-16 01:40:07 +00:00
Author
Owner
Copy link

Oban docs say by default job uniqueness is checked across all states except :discarded and :cancelled

oh this may actually be an issue - we don't actually enable the unique check for any worker 🥴

maybe that should also get enabled as part of this, i'll make sure it works

>Oban docs say by default job uniqueness is checked across all states except :discarded and :cancelled oh this may actually be an issue - we don't actually enable the `unique` check for any worker 🥴 maybe that should also get enabled as part of this, i'll make sure it works
floatingghost added 1 commit 2024-04-16 01:54:25 +00:00
Enable oban job uniqueness
Some checks are pending
ci/woodpecker/push/build-amd64 Pipeline is pending
Details
ci/woodpecker/push/build-arm64 Pipeline is pending
Details
ci/woodpecker/push/docs Pipeline is pending
Details
ci/woodpecker/push/lint Pipeline is pending
Details
ci/woodpecker/push/test Pipeline is pending
Details
ci/woodpecker/pr/build-amd64 Pipeline is pending
Details
ci/woodpecker/pr/build-arm64 Pipeline is pending
Details
ci/woodpecker/pr/docs Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline is pending
Details
ci/woodpecker/pr/test Pipeline is pending
Details
5043571084 
by default just prevent job floods with a 1-seconds
uniqueness check, but override in RemoteFetcherWorker
for 5 minute uniqueness check over all states

:infinity is an option we can go for maybe at some point,
but that would prevent any refetches so maybe not idk.
floatingghost added 1 commit 2024-04-16 01:59:02 +00:00
oban options should be a keyword list
Some checks failed
ci/woodpecker/push/build-amd64 Pipeline is pending
Details
ci/woodpecker/push/build-arm64 Pipeline is pending
Details
ci/woodpecker/push/docs Pipeline is pending
Details
ci/woodpecker/push/lint Pipeline is pending
Details
ci/woodpecker/push/test Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline failed
Details
ci/woodpecker/pr/test unknown status
Details
ci/woodpecker/pr/build-arm64 unknown status
Details
ci/woodpecker/pr/build-amd64 unknown status
Details
ci/woodpecker/pr/docs unknown status
Details
d70fa16383
floatingghost added 1 commit 2024-04-16 02:07:36 +00:00
mix format says no
Some checks failed
ci/woodpecker/push/build-amd64 Pipeline is pending
Details
ci/woodpecker/push/build-arm64 Pipeline is pending
Details
ci/woodpecker/push/docs Pipeline is pending
Details
ci/woodpecker/push/lint Pipeline is pending
Details
ci/woodpecker/push/test Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/test Pipeline was successful
Details
ci/woodpecker/pr/build-arm64 unknown status
Details
ci/woodpecker/pr/docs unknown status
Details
ci/woodpecker/pr/build-amd64 unknown status
Details
d2cee15c15
floatingghost commented 2024-04-16 02:25:28 +00:00
Author
Owner
Copy link

marinating on ihba to see if this breaks, prayge it does not

marinating on ihba to see if this breaks, prayge it does not
floatingghost added 1 commit 2024-04-16 09:19:45 +00:00
make xmerl shut up about markup
Some checks failed
ci/woodpecker/push/build-amd64 Pipeline is pending
Details
ci/woodpecker/push/build-arm64 Pipeline is pending
Details
ci/woodpecker/push/docs Pipeline is pending
Details
ci/woodpecker/push/lint Pipeline is pending
Details
ci/woodpecker/push/test Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/test Pipeline failed
Details
ci/woodpecker/pr/build-amd64 unknown status
Details
ci/woodpecker/pr/build-arm64 unknown status
Details
ci/woodpecker/pr/docs unknown status
Details
b2c29527fb
floatingghost added 1 commit 2024-04-16 11:59:29 +00:00
Merge branch 'develop' into failed-fetch-processing
Some checks failed
ci/woodpecker/push/build-amd64 Pipeline is pending
Details
ci/woodpecker/push/build-arm64 Pipeline is pending
Details
ci/woodpecker/push/docs Pipeline is pending
Details
ci/woodpecker/push/lint Pipeline is pending
Details
ci/woodpecker/push/test Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/test Pipeline was successful
Details
ci/woodpecker/pr/build-amd64 unknown status
Details
ci/woodpecker/pr/build-arm64 unknown status
Details
ci/woodpecker/pr/docs unknown status
Details
123db1abc4
Oneric reviewed 2024-04-17 14:13:57 +00:00
lib/pleroma/workers/remote_fetcher_worker.ex Outdated
@ -8,1 +8,3 @@
use Pleroma.Workers.WorkerHelper, queue: "remote_fetcher"
use Pleroma.Workers.WorkerHelper,
queue: "remote_fetcher",
unique: [period: 300, states: Oban.Job.states()]
Oneric commented 2024-04-17 14:13:57 +00:00
Member
Copy link

Multiple fetches of the same AP id can still occur if the depth arg differs; setting keys to only consider op and id should avoid this

Multiple fetches of the same AP id can still occur if the `depth` arg differs; setting `keys` to only consider `op` and `id` should avoid this
floatingghost added 1 commit 2024-04-19 10:39:41 +00:00
only consider :op and :id args in duplicate checks
Some checks failed
ci/woodpecker/push/build-amd64 Pipeline is pending
Details
ci/woodpecker/push/build-arm64 Pipeline is pending
Details
ci/woodpecker/push/docs Pipeline is pending
Details
ci/woodpecker/push/lint Pipeline is pending
Details
ci/woodpecker/push/test Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/test Pipeline was successful
Details
ci/woodpecker/pr/build-amd64 unknown status
Details
ci/woodpecker/pr/build-arm64 unknown status
Details
ci/woodpecker/pr/docs unknown status
Details
370576474c
floatingghost commented 2024-04-19 11:25:07 +00:00
Author
Owner
Copy link

has been running on IHBA with no noticable negative effects

has been running on IHBA with no noticable negative effects
🎉 1
floatingghost merged commit 0fee71f58f into develop 2024-04-19 11:25:14 +00:00
floatingghost deleted branch failed-fetch-processing 2024-04-19 11:25:15 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
approved, awaiting change

   
bug

Something is not working

  
configuration

This requires config changes

  
documentation

This is about human-readable docs

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
extremely low priority

   
feature request

Something new?

  
Fix it yourself

Support has been attempted

  
help wanted

Need some help

  
invalid

Something is wrong

  
mastodon_api

   
needs docs

   
needs tests

   
not a bug

   
planned

   
pleroma_api

   
privacy

   
question

More information is needed

  
static_fe

   
triage

   
wontfix

This won't be fixed

No labels
approved, awaiting change
bug
configuration
documentation
duplicate
enhancement
extremely low priority
feature request
Fix it yourself
help wanted
invalid
mastodon_api
needs docs
needs tests
not a bug
planned
pleroma_api
privacy
question
static_fe
triage
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: AkkomaGang/akkoma#743
No description provided.
Delete branch "failed-fetch-processing"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?