When not preserving job records, ensure all prior executions are deleted after successful retry

[ylansegal]

When records are configured to be preserved on on_unhandled_error, and later retried, the previous executions stay in the database and become "orphaned". I believe they should be deleted on a successful retry. This would bring it inline with the behavior for destroying a failed job (eg. via the UI) which results in previous executions also being deleted.

This is my first contribution to this repo. I took a stab at writing a spec that exposes the issue and a fix via a ActiveRecord callback. I am open to suggestions for improvement.

Thank you for making good_job available and open source!

[bensheldon]

@ylansegal thank you for opening up this PR. I think you've discovered a bug 🎉

I need to think about this a bit more. I don't think the fix here is quite right because I think if a a job is retried (which will generate a new execution) this PR will delete that new execution, which isn't desired.

Here is where the Execution#destroy is called after performing the job (the subsequent execution gets created/enqueued as part of the ActiveJob performance):

good_job/app/models/good_job/execution.rb

Lines 294 to 306 in e13915e

	result = execute
	job_error = result.handled_error || result.unhandled_error
	self.error = [job_error.class, ERROR_MESSAGE_SEPARATOR, job_error.message].join if job_error
	if result.unhandled_error && GoodJob.retry_on_unhandled_error
	save!
	elsif GoodJob.preserve_job_records == true || (result.unhandled_error && GoodJob.preserve_job_records == :on_unhandled_error)
	self.finished_at = Time.current
	save!
	else
	destroy!
	end

I'm thinking this logic needs to be unrolled to more clearly separate "This is what happened with the job/execution (e.g. succeeded, retried, discarded/handled, unhandled" and then within that have the logic of "and based on the configuration, this is what should happen with the record". I might push up into your branch unless you want to dig into that more.

[ylansegal]

I need to think about this a bit more. I don't think the fix here is quite right because I think if a a job is retried (which will generate a new execution) this PR will delete that new execution, which isn't desired.

I definitely had not considered retries fully. My thinking was that failed jobs are preserved, which would not trigger the callback. That seems to be an assumption on how I have configured GoodJob in my project.

I'm thinking this logic needs to be unrolled to more clearly separate "This is what happened with the job/execution (e.g. succeeded, retried, discarded/handled, unhandled" and then within that have the logic of "and based on the configuration, this is what should happen with the record". I might push up into your branch unless you want to dig into that more.

Feel free to push to that branch! You definitely have more knowledge of the internals.

[bensheldon]

Released in v3.4.8 🎉 Thank you for the fix 🙌🏻

[bensheldon]

@ylansegal sorry, I need to revert this because of an unexpected issue in #728. I will figure out a better implementation and get this behavior re-added.

[ylansegal]

Thank you @bensheldon