lardawge/carrierwave_backgrounder

When destroy a model, same queue is repeatedly processed

Closed this issue · 11 comments

When I destroyed a model, I encountered the problem that same sidekiq queue was repeatedly processed in a very short time, and finally sidekiq.log file became very huge size.
I thought this was because enqueue_#{column}_background_job was kicked by after_commit, enqueued my resize job, my resize job always failed and called when_not_ready because record not found, and retried the same job repeatedly.
My resize job is like this (same as README.md)

class MyResizeJobWorker < ActiveJob::Base
  include ::CarrierWave::Workers::ProcessAssetMixin

  def when_not_ready
    retry_job
  end
end

I know I can avoid this problem by deleting retry_job method from my resize job class.
But I think enqueing callback should be kicked only when record was saved or updated, not destroyed.
What do you think about it?

I'm also experiencing this. After looking at the code it's basically just swallowing the exceptions that ActiveRecord / Mongoid are throwing

https://github.com/lardawge/carrierwave_backgrounder/blob/master/lib/backgrounder/workers/base.rb#L16

janko commented

After looking at the code it's basically just swallowing the exceptions that ActiveRecord / Mongoid are throwing

@kyledecot Isn't that the correct behaviour? If versions start processing, and the record gets deleted in the meanwhile, then the background job shouldn't raise an error, right? I mean, there is nothing that the background job can do at that point (except maybe delete the processed files, since they won't be used anymore).

@janko-m perhaps but then how would one handle the case this issue describes wherein the record is destroyed before the background job is processed (resulting in an infinite loop of retries when using the when_not_ready hook). This issue in particular caused an enormous spike in redis keys being set which is the only way that I noticed the behavior in the first place as the exceptions were being swallowed.

janko commented

@kyledecot Oh, I understand what you mean now. It's surprising that carrierwave_backgrounder has this behaviour, since the background job is spawned in an after_commit hook, which means that the record will definitely be there in the background job. And if it's not there, then it must mean that it was deleted, like it is in your case, and then it will never be present again.

From time to time I keep checking this repo to see if there is something that I might have missed in Shrine, but it's strange to see carrierwave_backgrounder (and delayed_paperclip) still struggling with things that I already solved in Shrine in the early days. Not saying that it's easy, it was definitely very hard and took a lot of time, but I knew I had to solve these problems if I wanted backgrounding to be reliable. Just putting it out there in case it pokes somebody's interest.

@kyledecot How did you stop it? I have a process running shit loads of jobs from a deleted resource...

@davidwessman I basically just re-raised the exception:

class AvatarJob < ActiveJob::Base
  include ::CarrierWave::Workers::ProcessAssetMixin

  queue_as :carrierwave

  def when_not_found
    raise ActiveRecord::RecordNotFound
  end

  def when_not_ready
    retry_job
  end
end

I see that you've submitted a pull request to set the default job worker to retry_job on when_not_ready. This however was the source of my problem as a deleted avatar in my case would never be ready and it resulted in an infinite queueing of jobs.

👍 @kyledecot

Thank you for telling me!
Where did you find the when_not_found method?

@davidwessman I forked the project and implemented it myself (#248) . So far it's been working well enough but it definitely doesn't seem like the most elegant solution.

Oh! @kyledecot
Would be good as a feature...

The issue still persists. Right now i have an issue when deleting images that are still processed and I find that the only way to prevent this is to query the sidekiq queue, find the job by class and args and delete. Really slow and not recommended probably. Has anyone found a way around this?

I know this is old but want to update in case this comes up via a google search.

I am preparing to release a 1.0 version of this in the coming week. In doing so, I am attempting to add support for ActiveJob natively. This has led me down the rabbit hole of trying to understand what the idea behind the when_not_ready hook was used for.

Here's the deal, there is no use case for it. We are using after_commit. This means that by the time the job runs, there should be no issue with running it. Please chime in if I am missing a use case.

In the case where the record does not exist, it should fail silently and not retry, which it currently does.

There is a 1.0-beta branch for the curious. As always, I advise you to read through the docs first. There have been a few things removed that are just not useful at the moment. The idea here is to make it dead simple to process and store files in the background.

Also, it should be much easier to test new features as I have added a rails app in the spec directory to test against.