Handle missing parent when deleting comment

[jhass]

When a post is deleted, a comment retraction might race with a post retraction,
causing the comment deletion to raise because of a missing parent.

Fixes

NoMethodError: undefined method `update_comments_counter' for nil:NilClass
  from app/models/comment.rb:50:in `block in <class:Comment>'
  from lib/diaspora/federation/receive.rb:177:in `retraction'
  from config/initializers/diaspora_federation.rb:112:in `block (3 levels) in <top (required)>'
  from app/workers/receive_private.rb:13:in `block in perform'
  from app/workers/receive_base.rb:11:in `filter_errors_for_retry'
  from app/workers/receive_private.rb:10:in `perform'

[SuperTux88]

I'm a bit confused, when does this happen? The stacktrace says it's failing here:

diaspora/lib/diaspora/federation/receive.rb

Line 177 in d5b7c6d

object.destroy!

But deleting a comment would go here:

diaspora/lib/diaspora/federation/receive.rb

Lines 167 to 175 in d5b7c6d

	when Diaspora::Relayable
	if object.root.author.local?
	root_author = object.root.author.owner
	retraction = Retraction.for(object)
	retraction.defer_dispatch(root_author, false)
	retraction.perform
	else
	object.destroy!
	end

So do we receive a retraction for the post which then destroys all comments which then fails? If this fails, I would expect a bigger problem, because then probably no post with comment can't be deleted?

Or do we receive a rectraction for both, a comment and it's parent at the same time? But then I'm not sure if this really fixes it, or if it again fails later because deleting the parent also deletes all it's comment, so a comment can't be deleted twice?

[jhass]

Well, honestly I don't know.

Or do we receive a rectraction for both, a comment and it's parent at the same time? But then I'm not sure if this really fixes it, or if it again fails later because deleting the parent also deletes all it's comment, so a comment can't be deleted twice?

Seems the more likely scenario to me and that the parent and comment retraction actually run at the same time, so this being a race condition, which is why earlier checks may pass.

Either way I could write a spec for this so this is a potential state the system can be in, right? We have no database constraints preventing this, so we should handle it IMO if for nothing but fault tolerance.

If it fails differently down the line again we'll get the next stacktrace and can handle it then :)