-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Handle processing a mention in a comment of an already deleted post #8057
base: develop
Are you sure you want to change the base?
Handle processing a mention in a comment of an already deleted post #8057
Conversation
jhass
commented
Sep 11, 2019
This is a similar case as #8056, and I don't think that's a good solution here. It adds a fix (and complexity) at a place where it doesn't belong. This case should never happen (and I can find this error neither on my pod nor on the hq pod), when the parent post is deleted, it deletes all comments on the post and new comments can't be created anymore. Yes, in theory, it is possible that a comment is loaded right before it's deleted and then tried to do stuff with it. But should we really add extra checks at all places that the parent is there (and it would also need checks if the comment is still there, because this would try to create more than just the mention notification which then try to link to the non-existing parent and comment)? I this case: When the parent and the comment isn't there anymore, this would fail once with the error above, then go back to retry queue in sidekiq. When it then is retried, it can't find the comment anymore and throws an If this really is a onetime-error (since I can't find it on my pods), I would probably just ignore it (but that's only my opinion, @denschub already approved it). If that happens more often on your pod, we should check why this happens, because then something much bigger is broken which would need a fix elsewhere. |
I've seen this 13 times so far. It's super low-frequency, but I figured there probably is no harm in avoiding it either since this patch adds no real cost to anything... |
13 times since your pod exists? It doesn't add performance cost, but it makes But it's also not the end of the world, so ¯\_(ツ)_/¯ |
Interesting, that shouldn't happen. Because when the parent doesn't exist anymore, the comment also shouldn't exist anymore and at least the first retry it should "fail" at diaspora/app/workers/receive_local.rb Line 8 in e826909
with a Maybe the first part of the receive (the actual receiving of the comment and creating it in the database) did run parallel to the delete of the parent post? So it managed to create the comment, but the parent was already destroying all it's child-comments and it didn't have the new comment in the list yet? I don't know if there is a good way to catch this? Maybe check if parents still exist after receive was complete? But we would need to do this for more than just the comments. Comment parents are polymorphic, that's why we can't enforce it on a database level and it also means that the same can happen for other things that use polymorphic parents (likes for example). For comments, we could just remove the polymorphic parent, since only posts can have comments. For likes we have both posts and comments as targets, but maybe we could split them into two tables (which would make the likes-table smaller again), that way we could enforce problems like this on a database-level which would be a much safer solution and less hacky than any "check after receive if the parent is still there" (and it probably would have prevented this error even the first time). |
It is
|
The same thing just happened again on Geraspora, this time it's comment Probably also interesting to note:
|