`NotImplementedError` is always retried

[olivier-thatch]

NotImplementedError does not derive from StandardError. Per the documentation, exceptions that don't derive from StandardError are always retried, but if NotImplementedError is raised it's pretty much guaranteed that bad code was deployed and the job will never succeed.

We ran into this issue in our codebase, where a job was retried over 10,000 times before someone noticed and force-terminated the job via the UI. Also, the UI shows GoodJob::InterruptError as the exception, masking the actual NotImplementedError exception:

We've fixed this by adding discard_on NotImplementedError to the ApplicationJob parent class, but ideally I think GoodJob should know not to retry jobs on NotImplementedError exceptions.

[bensheldon]

It's tricky! NotImplementedError is the weird one. The problem becomes that if GoodJob automatically discards on NotImplementedError it still punts the problem into the application for what to do (not everyone will want it discarded, some will want it reported and retried exponentially).

I think the strong community opinion is not to use NotImplementedError at all (I think NoMethodError is preferred, but I see people arguing about it):

• https://nithinbekal.com/posts/abstract-methods-notimplementederror-ruby/
• https://web.archive.org/web/20180108211239/http://chrisstump.online/2016/03/23/stop-abusing-notimplementederror/
• https://oleg0potapov.medium.com/ruby-notimplementederror-dont-use-it-dff1fd7228e5

I feel like maybe the best we can do is document it ☹️

[olivier-thatch]

Regardless of whether NotImplementedError is used correctly (to signal that a feature is not available with the current runtime, e.g. trying to fork a process when using JRuby) or incorrectly (typically, to signal an abstract method that needs to be implemented by subclasses), I think the chances of the error going away on a retry are basically nil and it should be safe to discard by default.

That said, I agree that it's ultimately a user error and not GoodJob's responsibility. Documenting the behavior would be helpful though.

It would also be nice if the UI displayed the underlying error in some way rather than completely masking it under GoodJob::InterruptError.

[bensheldon]

It would also be nice if the UI displayed the underlying error in some way

The reason for this is an an Exception blows through the GoodJob stack entirely and nothing more gets written to the job's database record. It's only when the job is re-executed does GoodJob then clean it up. So all GoodJob knows is that there was some kind of Exception/interrupt, but not anything more precise.

I imagine there could be a configuration that was like non_interrupt_errors or something that could be modified to include additional exception classes other than StandardError:

good_job/app/models/good_job/job.rb

Line 675 in 07c4cf1

rescue StandardError => e