Handle processing a mention in a comment of an already deleted post

[jhass]

NoMethodError: undefined method `public?' for nil:NilClass
  from app/models/person.rb:141:in `block in <class:Person>'
  from app/models/notifications/mentioned_in_comment.rb:16:in `filter_mentions'
  from app/models/notifications/mentioned.rb:14:in `notify'
  from app/services/notification_service.rb:15:in `block in notify'
  from app/services/notification_service.rb:15:in `each'
  from app/services/notification_service.rb:15:in `notify'
  from app/workers/receive_local.rb:12:in `perform'

[SuperTux88]

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 ActiveRecord::RecordNotFound which marks the job as completed.

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.

[denschub]

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...

[SuperTux88]

13 times since your pod exists?

It doesn't add performance cost, but it makes allowed_to_be_mentioned_in_a_comment_to more complex with a case that (in theory) should never happen. And it probably doesn't fix the problem, since the mention-notification is only the first thing that happens for a comment, and even there it fails really early. I think if we fix it at that place, it probably still fails with other notifications or somewhere else (but didn't test it), and I don't know if it's worth to add a check everywhere.

But it's also not the end of the world, so ¯\_(ツ)_/¯

[jhass]

I got it basically once, that is for one specific comment which it didn't fail differently for in the retries. Maybe it's somehow possible to trigger by sending a specifically crafted comment after all?

[SuperTux88]

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

object = object_class_string.constantize.find(object_id)

with a ActiveRecord::RecordNotFound. Do you still know for which comment that was? Because it probably still exists in your DB, but the parent for it doesn't exist anymore, so we have inconsistent data here.

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).

[jhass]

It is

irb(main):001:0> Comment.find(34764)
=> #<Comment id: 34764, text: "My apologies everyone. Someone just shared this an...", commentable_id: 48769, author_id: 1129, guid: "ea604d30b6500137ac3279d7a726d437", created_at: "2019-09-10 23:30:36", updated_at: "2019-09-10 23:31:23", likes_count: 0, commentable_type: "Post", tag_list: nil>

[denschub]

The same thing just happened again on Geraspora, this time it's comment guid=e8bfd860dfc50137de9e00163e73147f. The comment's author is another diaspora* user on a 0.7.12.0 pod.

Probably also interesting to note:

Comment.joins("LEFT JOIN posts ON posts.id = comments.commentable_id").where("posts IS NULL").count
=> 26