Start discard from execution, not job

To avoid a race condition when we check whether we have a ready execution and the job
gets claimed right before, and after we've already checked if it was claimed. Rely
always on executions to delete a job that's not finished. When it's finished, it can
be cleared in bulk, without having to worry about the callbacks.

Author: rosa

app/models/solid_queue/claimed_execution.rb

@@ -44,6 +44,10 @@ def release
     end
   end
 
+  def discard
+    raise UndiscardableError, "Can't discard a job in progress"
+  end
+
   private
     def execute
       ActiveJob::Base.execute(job.arguments)

app/models/solid_queue/execution.rb

@@ -2,6 +2,8 @@
 
 module SolidQueue
   class Execution < Record
+    class UndiscardableError < StandardError; end
+
     include JobAttributes
 
     self.abstract_class = true
@@ -10,8 +12,6 @@ class Execution < Record
 
     belongs_to :job
 
-    alias_method :discard, :destroy
-
     class << self
       def create_all_from_jobs(jobs)
         insert_all execution_data_from_jobs(jobs)
@@ -23,5 +23,12 @@ def execution_data_from_jobs(jobs)
         end
       end
     end
+
+    def discard
+      with_lock do
+        job.destroy
+        destroy
+      end
+    end
   end
 end

app/models/solid_queue/job/concurrency_controls.rb

@@ -6,9 +6,11 @@ module ConcurrencyControls
       extend ActiveSupport::Concern
 
       included do
-        has_one :blocked_execution, dependent: :destroy
+        has_one :blocked_execution
 
         delegate :concurrency_limit, :concurrency_duration, to: :job_class
+
+        before_destroy :unblock_next_blocked_job, if: :ready?
       end
 
       def unblock_next_blocked_job
@@ -49,6 +51,10 @@ def release_next_blocked_job
         def job_class
           @job_class ||= class_name.safe_constantize
         end
+
+        def execution
+          super || blocked_execution
+        end
     end
   end
 end

app/models/solid_queue/job/executable.rb

@@ -8,9 +8,9 @@ module Executable
       included do
         include Clearable, ConcurrencyControls, Schedulable
 
-        has_one :ready_execution, dependent: :destroy
-        has_one :claimed_execution, dependent: :destroy
-        has_one :failed_execution, dependent: :destroy
+        has_one :ready_execution
+        has_one :claimed_execution
+        has_one :failed_execution
 
         after_create :prepare_for_execution
 
@@ -89,16 +89,14 @@ def finished?
         finished_at.present?
       end
 
-      def discard
-        unless claimed?
-          try_to_discard_while_ready || destroy
-        end
-      end
-
       def retry
         failed_execution&.retry
       end
 
+      def discard
+        execution.discard
+      end
+
       def failed_with(exception)
         FailedExecution.create_or_find_by!(job_id: id, exception: exception)
       end
@@ -108,15 +106,10 @@ def ready
           ReadyExecution.create_or_find_by!(job_id: id)
         end
 
-        def try_to_discard_while_ready
-          # Prevent the job from being polled while being discarded
-          ready_execution&.with_lock do
-            unblock_next_blocked_job
-            destroy
-          end
+        def execution
+          %w[ ready claimed failed ].reduce(nil) { |acc, status| acc || public_send("#{status}_execution") }
         end
 
-
         def preserve_finished_jobs?
           SolidQueue.preserve_finished_jobs
         end

app/models/solid_queue/job/schedulable.rb

@@ -6,7 +6,7 @@ module Schedulable
       extend ActiveSupport::Concern
 
       included do
-        has_one :scheduled_execution, dependent: :destroy
+        has_one :scheduled_execution
 
         scope :scheduled, -> { where.not(finished_at: nil) }
       end
@@ -35,6 +35,10 @@ def due?
         def schedule
           ScheduledExecution.create_or_find_by!(job_id: id)
         end
+
+        def execution
+          super || scheduled_execution
+        end
     end
   end
 end

app/models/solid_queue/ready_execution.rb

@@ -37,5 +37,12 @@ def lock_candidates(job_ids, process_id)
           end
         end
     end
+
+    def discard
+      with_lock do
+        job.destroy
+        destroy
+      end
+    end
   end
 end

test/models/solid_queue/job_test.rb

@@ -170,7 +170,9 @@ class NonOverlappingGroupedJob2 < NonOverlappingJob
     sleep(0.2)
 
     assert_no_difference -> { SolidQueue::Job.count }, -> { SolidQueue::ClaimedExecution.count } do
-      job.discard
+      assert_raises SolidQueue::Execution::UndiscardableError do
+        job.discard
+      end
     end
 
     worker.stop
@@ -225,6 +227,7 @@ class NonOverlappingGroupedJob2 < NonOverlappingJob
   private
     def assert_ready(&block)
       assert_job_counts(ready: 1, &block)
+      assert SolidQueue::Job.last.ready?
     end
 
     def assert_scheduled(&block)
@@ -233,6 +236,7 @@ def assert_scheduled(&block)
 
     def assert_blocked(&block)
       assert_job_counts(blocked: 1, &block)
+      assert SolidQueue::Job.last.blocked?
     end
 
     def assert_job_counts(ready: 0, scheduled: 0, blocked: 0, &block)