Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deletes not reaching every server that interacted with status #15200

Merged
merged 1 commit into from
Nov 27, 2020

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Nov 23, 2020

Extract logic for determining ActivityPub inboxes to send deletes to to its own class and explicitly include the person the status
replied to (even if not mentioned), people who favourited it, and people who replied to it (though that one is still not recursive)

@Gargron Gargron added the activitypub Protocol-related changes, federation label Nov 23, 2020
@ClearlyClaire
Copy link
Contributor

I'm ok with the general idea, and factoring it out looks cleaner and is easier to reason about. As for the failing test, I have to admit I have no clue what's going on, for some reason I don't understand, @status.reblogs is empty even though a reblog has been instantiated.

It should also be noted that instances that fetched the toot in any way without actually interacting with it won't necessarily get a delete, and it mostly relies on intermediary instances forwarding the Delete. But it should be an improvement over the status quo nonetheless.

@Gargron Gargron force-pushed the fix-status-delete-reach branch from 6951d54 to 35b880c Compare November 27, 2020 01:57
@Gargron
Copy link
Member Author

Gargron commented Nov 27, 2020

The unit test caught a real issue. Reblogs were removed before StatusReachFinder was called, this was fine before because reblogs were preloaded into an instance variable. But I see no reason why the order of execution should be that way anymore, so I changed it. Also, since the preloads were no longer used more than once in the class, I replaced them with potentially more efficient find_each calls.

@Gargron Gargron force-pushed the fix-status-delete-reach branch from 35b880c to 43f78d6 Compare November 27, 2020 02:42
@Gargron
Copy link
Member Author

Gargron commented Nov 27, 2020

I also identified some inefficies. When removing a reblog, the class would make queries for the reblog's mentions, media attachments, tags, etc. Since reblogs don't possess those, we can cut down on those queries. Also, due to using pluck on the tags, the tags actually got queried twice, once the IDs for featured tags, and then the names for the timelines.

@Gargron Gargron force-pushed the fix-status-delete-reach branch 2 times, most recently from 8213d6a to 1542816 Compare November 27, 2020 02:45
Extract logic for determining ActivityPub inboxes to send deletes
to to its own class and explicitly include the person the status
replied to (even if not mentioned), people who favourited it, and
people who replied to it (though that one is still not recursive)
@Gargron Gargron force-pushed the fix-status-delete-reach branch from 1542816 to caaca9a Compare November 27, 2020 02:54
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch with the reblogs stuff. This seems better overall, but there's one thing I'm not sure about (though it shouldn't be a big issue), see inline comments.

Comment on lines +80 to 82
@status.active_mentions.find_each do |mention|
redis.publish("timeline:#{mention.account_id}", @payload)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this is to avoid fetching account information from the database, but this will send events about remote accounts to redis. Is it worth it? Does it make sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right... but publishing to a channel with no subscribers should be O(0) so probably fine this way.

@Gargron Gargron merged commit e7e099d into master Nov 27, 2020
@Gargron Gargron deleted the fix-status-delete-reach branch November 27, 2020 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activitypub Protocol-related changes, federation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants