When not preserving job records, ensure all prior executions are dele…

…ted after successful retry (bensheldon#719)

* Ensure prior executions are deleted on successful retry

* Destroy execution records only if the job has not been retried; add targeted method for destroy

* Add unit test for destroy_job

Co-authored-by: Ben Sheldon [he/him] <[email protected]>

app/models/good_job/execution.rb

@@ -66,6 +66,7 @@ def self.queue_parser(string)
     end
 
     belongs_to :job, class_name: 'GoodJob::Job', foreign_key: 'active_job_id', primary_key: 'active_job_id', optional: true, inverse_of: :executions
+    after_destroy -> { self.class.active_job_id(active_job_id).delete_all }, if: -> { @_destroy_job }
 
     # Get Jobs with given ActiveJob ID
     # @!method active_job_id
@@ -298,11 +299,11 @@ def perform
 
       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)
+      elsif GoodJob.preserve_job_records == true || result.retried? || (result.unhandled_error && GoodJob.preserve_job_records == :on_unhandled_error)
         self.finished_at = Time.current
         save!
       else
-        destroy!
+        destroy_job
       end
 
       result
@@ -354,6 +355,14 @@ def runtime_latency
       (finished_at || Time.zone.now) - performed_at if performed_at
     end
 
+    # Destroys this execution and all executions within the same job
+    def destroy_job
+      @_destroy_job = true
+      destroy!
+    ensure
+      @_destroy_job = false
+    end
+
     private
 
     def active_job_data
@@ -379,7 +388,7 @@ def execute
           end
           handled_error ||= current_thread.error_on_retry || current_thread.error_on_discard
 
-          ExecutionResult.new(value: value, handled_error: handled_error)
+          ExecutionResult.new(value: value, handled_error: handled_error, retried: current_thread.error_on_retry.present?)
         rescue StandardError => e
           ExecutionResult.new(value: nil, unhandled_error: e)
         end

app/models/good_job/execution_result.rb

@@ -8,14 +8,18 @@ class ExecutionResult
     attr_reader :handled_error
     # @return [Exception, nil]
     attr_reader :unhandled_error
+    # @return [Exception, nil]
+    attr_reader :retried
+    alias retried? retried
 
     # @param value [Object, nil]
     # @param handled_error [Exception, nil]
     # @param unhandled_error [Exception, nil]
-    def initialize(value:, handled_error: nil, unhandled_error: nil)
+    def initialize(value:, handled_error: nil, unhandled_error: nil, retried: false)
       @value = value
       @handled_error = handled_error
       @unhandled_error = unhandled_error
+      @retried = retried
     end
   end
 end

lib/good_job.rb

@@ -57,7 +57,7 @@ module GoodJob
   #   By default, GoodJob deletes job records after the job is completed successfully.
   #   If you want to preserve jobs for latter inspection, set this to +true+.
   #   If you want to preserve only jobs that finished with error for latter inspection, set this to +:on_unhandled_error+.
-  #   @return [Boolean, nil]
+  #   @return [Boolean, Symbol, nil]
   mattr_accessor :preserve_job_records, default: true
 
   # @!attribute [rw] retry_on_unhandled_error

spec/app/models/good_job/execution_spec.rb

@@ -397,6 +397,25 @@ def job_params
       expect { good_job.reload }.to raise_error ActiveRecord::RecordNotFound
     end
 
+    context 'when there are prior executions' do
+      let!(:prior_execution) do
+        described_class.enqueue(active_job).tap do |job|
+          job.update!(
+            error: "TestJob::ExpectedError: Raised expected error",
+            performed_at: Time.current,
+            finished_at: Time.current
+          )
+        end
+      end
+
+      it 'destroys the job and prior executions when preserving record only on error' do
+        allow(GoodJob).to receive(:preserve_job_records).and_return(:on_unhandled_error)
+        good_job.perform
+        expect { good_job.reload }.to raise_error ActiveRecord::RecordNotFound
+        expect { prior_execution.reload }.to raise_error ActiveRecord::RecordNotFound
+      end
+    end
+
     it 'raises an error if the job is attempted to be re-run' do
       good_job.update!(finished_at: Time.current)
       expect { good_job.perform }.to raise_error described_class::PreviouslyPerformedError
@@ -504,6 +523,19 @@ def job_params
     end
   end
 
+  describe '#destroy_job' do
+    let!(:execution) { described_class.create! active_job_id: SecureRandom.uuid }
+    let!(:prior_execution) { described_class.create! active_job_id: execution.active_job_id }
+    let!(:other_job) { described_class.create! active_job_id: SecureRandom.uuid }
+
+    it 'destroys all of the job executions' do
+      execution.destroy_job
+      expect { execution.reload }.to raise_error ActiveRecord::RecordNotFound
+      expect { prior_execution.reload }.to raise_error ActiveRecord::RecordNotFound
+      expect { other_job.reload }.not_to raise_error
+    end
+  end
+
   describe '#queue_latency' do
     let(:execution) { described_class.create! }
 

spec/test_app/config/environments/test.rb

@@ -4,8 +4,8 @@
 # and recreated between test runs. Don't rely on the data there!
 
 Rails.application.configure do
-  config.cache_classes = false
-  config.eager_load = false
+  config.cache_classes = true
+  config.eager_load = true
 
   # Raises error for missing translations.
   if Gem::Version.new(Rails.version) < Gem::Version.new('6.1')