Generate post thumbnail/metadata in background (ref #4529) #4564

Nutomic · 2024-03-25T12:29:37Z

@ticoombs mentioned that its sometimes extremely slow to generate the post thumbnail and metadata because sites are slow to respond. So Im moving this to a background task which also gets rid of some duplicate code.

I also noticed that the custom post thumbnail added in #4425 isnt working properly because its not federated. So other instances will generate a thumbnail as usual. Ive removed the custom thumbnail code for now.

dessalines · 2024-03-25T20:09:24Z

I also noticed that the custom post thumbnail added in #4425 isnt working properly because its not federated. So other instances will generate a thumbnail as usual. Ive removed the custom thumbnail code for now.

Why is that? That field is only added via the API, the thumbnail_url field should still be used for federation.

dessalines

Not sure I agree with backgrounding this. This means that front ends might think that there's a bug with missing picture / metadata after a post has been created, when in reality its just waiting for it, and they'll need to keep clicking refresh to make sure it fetched everything.

Also, not sure how this might affect federation of pics and metadata.

Nutomic · 2024-03-26T12:37:45Z

Why is that? That field is only added via the API, the thumbnail_url field should still be used for federation.

Sorry youre right, Ive restored it again.

Not sure I agree with backgrounding this. This means that front ends might think that there's a bug with missing picture / metadata after a post has been created, when in reality its just waiting for it, and they'll need to keep clicking refresh to make sure it fetched everything.

The problem is mentioned here. Basically to keep up with a major instance like lemmy.world, an instance has to process around 3 activities per second. But if its blocked fetching link metadata for 2 seconds, it will fall behind and spend the next seconds catching up. Thats a problem when an instance already has trouble keeping up.

Also, not sure how this might affect federation of pics and metadata.

Right that was a problem, but Ive moved it so its done after metadata generation now.

Nutomic · 2024-03-26T14:50:12Z

Theres a weird test failure where alt_text isnt getting federated. Its only failing sometimes, and despite me not changing anything related to alt text.

dessalines · 2024-03-26T14:57:15Z

crates/api_common/src/request.rs

+      ..Default::default()
+    };
+    let updated_post = Post::update(&mut context.pool(), post.id, &form).await?;
+    if let Some(send_activity) = send_activity(updated_post) {


dessalines · 2024-03-26T15:01:58Z

api_tests/src/post.spec.ts

+  // so wait for it explicitly. For removed posts we cant refetch anything.
+  postOne = await waitForPost(beta, postOne.post, res => {
+    return res == null || res?.post.embed_title != null;
+  });


The alt_text issue might be a logical problem here. Its returning even if the res is null.

The failing test is "No image proxying if setting is disabled" which doesnt use this function. The null check is necessary because theres a removed post which cant be fetched.

hrm... gotta be somewhere else then. Otherwise all the other PRs would have alt_text issues too, because that's already in main.

It seems to happen specifically with this PR, so be careful with merging even if CI randomly passes.

Nutomic · 2024-03-27T10:40:50Z

api_tests/src/image.spec.ts

+    post.post_view.post,
+    res => res?.post.alt_text != null,
+  );
+  expect(betaPost.post).toBeDefined();


Found the problem, gamma was fetching the post here before metadata was generated. This means the post url was federated as Link which doesnt include alt text. Have to subscribe to the community and wait for it to send the post out after metadata is generated. Also had to change from delta to beta because follow didnt go through from delta for some reason.

SleeplessOne1917 · 2024-03-27T12:11:57Z

api_tests/src/image.spec.ts

+  let betaPost = await waitForPost(
+    beta,
+    post.post_view.post,
+    res => res?.post.alt_text != null,


Suggested change

res => res?.post.alt_text != null,

res => res?.post.alt_text !== null,

This test fails with !== but passes with !=. Not sure why but Ive changed it back.

api_tests/src/post.spec.ts

crates/api_common/src/request.rs

Co-authored-by: SleeplessOne1917 <28871516+SleeplessOne1917@users.noreply.github.com>

kroese · 2024-04-05T00:42:10Z

@Nutomic @dessalines Excuse me if Im overlooking something, but why is it necessary to fetch metadata for federated posts at all?

I always assumed the metadata fetching was only done by the instance that generated the post. And since those fields (like thumbnail_url) are already federated, what is the purpose to let every receiving instance repeat that process? If the number of instances grows, that would trigger a DoS attack at the external URL by all those instances fetching the metadata at roughly the same time.

If the metadata fetching is necessary because some field is not federated, wouldnt it make much more sense to include that field in the post info instead of letting every server retrieve it from the external URL?

Nutomic · 2024-04-05T10:50:00Z

The metadata is not federated, and Im not entirely sure how that would be done over Activitypub. Looking at Mastodon they also dont federate it so it cant be that bad. Plus a malicious instance could send wrong metadata.

kroese · 2024-04-05T11:04:53Z

I am really surprised! I always thought my Lemmy instance was only making connections to other instances, but this means its constantly making GET requests to all kinds of non-Lemmy webservers for scraping.

I am not sure if Im too happy about that, for example Reddit may rate-limit my IP, other websites might block me for constant scraping. And it might connect to all kinds of shady or illegal websites.

Can you give one example why this metadata fetching is necessary? The thumbnail URL is already provided (which I hotlink). The only other thing seems to be that short text summary, which would be nice to have, but in no way critical.

Could there at least be an option to disable metadata scraping for federated posts? I dont mind my server connecting to other Lemmy servers, I just want it to trust the received metadata instead of crawling the whole web. That it does this for locally created posts is totally fine (and unavoidable), but for federated posts is overkill.

Nutomic · 2024-04-05T11:14:22Z

Sure you can open an issue for an option to disabling post metadata fetching.

kroese · 2024-04-05T11:31:56Z

Done, see #4598

kroese · 2024-04-23T08:55:44Z

@Nutomic I am looking into this commit, as I expected the background fetch would only apply to incoming (federated) posts. But if Im reading the code correctly, it seems it is also applied to outgoing (local) posts..

Is there any need to do that? Because this can potentially cause loads of issues (for example posts that already get federated to other instances before they were "finished", users that see their post incomplete before its finished, etc.).

It would make more sense to only apply the background fetching to incoming (federated) posts, where its effects will be much more harmless.

dessalines · 2024-04-24T03:22:41Z

I did test it locally, and it seemed to work fine. As you can see in the code here, after its done fetching, it sends the update over federation.

kroese · 2024-04-24T04:23:55Z

@dessalines Okay, good to know!

But that still means that every post (except the few ones without metadata) will be federated twice. Once during creation, and once for the update. Thus essentially doubling the load on the network, with no real benefit.

dessalines · 2024-04-24T14:44:38Z

True, its less than ideal, but we don't know another way to prevent blocking federation worker threads.

kroese · 2024-04-24T18:03:18Z

@dessalines But the blocking of federation worker threads is only applicable to incoming posts, for those this change was very useful, and I dont question that decision at all.

My complaint was that this change is also applied to outgoing posts. Since the outgoing posts are not in a queue at all, processing them asynchronously provides no benefit, only downsides (like doubling the network load). So this makes the federation queue even fuller.

To say it in other words: I understand why crates/apub/src/objects/post.rs was modified, and agree about that.

I just dont think it was smart that the same was done to crates/api_crud/src/post/create.rs and crates/api_crud/src/post/update.rs

Nutomic · 2024-04-24T21:56:50Z

Local posts are still only federated once, as you can see that ActivityChannel::submit_activity was moved into generate_post_link_metadata and is still only called once per API call. Doing this in the background does make sense for local posts, as it avoids long delay or timeout on the api call when metadata fetching is slow. The only benefit is that metadata and thumbnails may be missing for the first few seconds, but thats a very minor complaint.

Nutomic requested review from dessalines, phiresky, dullbananas and SleeplessOne1917 as code owners March 25, 2024 12:29

Nutomic mentioned this pull request Mar 25, 2024

[Bug]: Federation throughput in a synchronous manner is limited to network distance #4529

Open

5 tasks

dessalines reviewed Mar 25, 2024

View reviewed changes

Nutomic force-pushed the post-thumbnail-background branch from 9f1635b to 72ead46 Compare March 26, 2024 12:34

Generate post thumbnail/metadata in background (ref #4529)

33f7d08

Nutomic force-pushed the post-thumbnail-background branch from 72ead46 to 33f7d08 Compare March 26, 2024 12:39

dessalines reviewed Mar 26, 2024

View reviewed changes

fix api test

69aca19

Nutomic force-pushed the post-thumbnail-background branch from 9f85674 to 69aca19 Compare March 27, 2024 10:39

Nutomic commented Mar 27, 2024

View reviewed changes

Merge branch 'main' into post-thumbnail-background

8d3498d

SleeplessOne1917 reviewed Mar 27, 2024

View reviewed changes

Apply suggestions from code review

c275db4

Co-authored-by: SleeplessOne1917 <28871516+SleeplessOne1917@users.noreply.github.com>

SleeplessOne1917 approved these changes Mar 27, 2024

View reviewed changes

fix test

4154cc8

dessalines approved these changes Mar 27, 2024

View reviewed changes

dessalines enabled auto-merge (squash) March 27, 2024 14:47

dessalines merged commit a4b79ca into main Mar 27, 2024
2 checks passed

Nutomic mentioned this pull request Apr 8, 2024

[Feature] Disable metadata fetching for federated posts #4598

Open

5 tasks

SleeplessOne1917 deleted the post-thumbnail-background branch April 24, 2024 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate post thumbnail/metadata in background (ref #4529) #4564

Generate post thumbnail/metadata in background (ref #4529) #4564

Nutomic commented Mar 25, 2024

dessalines commented Mar 25, 2024

dessalines left a comment •

edited

Nutomic commented Mar 26, 2024 •

edited

Nutomic commented Mar 26, 2024

dessalines Mar 26, 2024

dessalines Mar 26, 2024

Nutomic Mar 26, 2024

dessalines Mar 26, 2024 •

edited

Nutomic Mar 26, 2024

Nutomic Mar 27, 2024

SleeplessOne1917 Mar 27, 2024

Nutomic Mar 27, 2024

kroese commented Apr 5, 2024 •

edited

Nutomic commented Apr 5, 2024 •

edited

kroese commented Apr 5, 2024 •

edited

Nutomic commented Apr 5, 2024

kroese commented Apr 5, 2024

kroese commented Apr 23, 2024

dessalines commented Apr 24, 2024

kroese commented Apr 24, 2024

dessalines commented Apr 24, 2024

kroese commented Apr 24, 2024

Nutomic commented Apr 24, 2024

	res => res?.post.alt_text != null,
	res => res?.post.alt_text !== null,

Generate post thumbnail/metadata in background (ref #4529) #4564

Generate post thumbnail/metadata in background (ref #4529) #4564

Conversation

Nutomic commented Mar 25, 2024

dessalines commented Mar 25, 2024

dessalines left a comment • edited

Choose a reason for hiding this comment

Nutomic commented Mar 26, 2024 • edited

Nutomic commented Mar 26, 2024

dessalines Mar 26, 2024

Choose a reason for hiding this comment

dessalines Mar 26, 2024

Choose a reason for hiding this comment

Nutomic Mar 26, 2024

Choose a reason for hiding this comment

dessalines Mar 26, 2024 • edited

Choose a reason for hiding this comment

Nutomic Mar 26, 2024

Choose a reason for hiding this comment

Nutomic Mar 27, 2024

Choose a reason for hiding this comment

SleeplessOne1917 Mar 27, 2024

Choose a reason for hiding this comment

Nutomic Mar 27, 2024

Choose a reason for hiding this comment

kroese commented Apr 5, 2024 • edited

Nutomic commented Apr 5, 2024 • edited

kroese commented Apr 5, 2024 • edited

Nutomic commented Apr 5, 2024

kroese commented Apr 5, 2024

kroese commented Apr 23, 2024

dessalines commented Apr 24, 2024

kroese commented Apr 24, 2024

dessalines commented Apr 24, 2024

kroese commented Apr 24, 2024

Nutomic commented Apr 24, 2024

dessalines left a comment •

edited

Nutomic commented Mar 26, 2024 •

edited

dessalines Mar 26, 2024 •

edited

kroese commented Apr 5, 2024 •

edited

Nutomic commented Apr 5, 2024 •

edited

kroese commented Apr 5, 2024 •

edited