Change the approach, destroying the job when it can't be enqueued

Otherwise we'd get inconsistent behaviour with other adapters like
Sidekiq, where only jobs that are actually enqueued get assigned a
provider_job_id. Turns out, issuing a DELETE query for a record that got
inserted within a transaction works perfectly, even with bulk INSERT, so
let's just avoid creating those jobs altogether.

Author: rosa

README.md

@@ -428,7 +428,7 @@ In the case of recurring tasks, if such error is raised when enqueuing the job c
 
 ## Concurrency controls
 
-Solid Queue extends Active Job with concurrency controls, that allows you to limit how many jobs of a certain type or with certain arguments can run at the same time. When limited in this way, by default, jobs will be blocked from running, and they'll stay blocked until another job finishes and unblocks them, or after the set expiry time (concurrency limit's _duration_) elapses. Alternatively, jobs can be configured to be discarded instead of blocked. This means that if a job with certain arguments has already been enqueued, other jobs with the same characteristics (in the same concurrency _class_) won't be enqueued, they'll silently complete.
+Solid Queue extends Active Job with concurrency controls, that allows you to limit how many jobs of a certain type or with certain arguments can run at the same time. When limited in this way, by default, jobs will be blocked from running, and they'll stay blocked until another job finishes and unblocks them, or after the set expiry time (concurrency limit's _duration_) elapses. Alternatively, jobs can be configured to be discarded instead of blocked. This means that if a job with certain arguments has already been enqueued, other jobs with the same characteristics (in the same concurrency _class_) won't be enqueued.
 
 ```ruby
 class MyJob < ApplicationJob
@@ -442,11 +442,17 @@ class MyJob < ApplicationJob
 - `group` is used to control the concurrency of different job classes together. It defaults to the job class name.
 - `on_conflict` controls behaviour when enqueuing a job that conflicts with the concurrency limits configured. It can be set to one of the following:
   - (default) `:block`: the job is blocked and is dispatched when another job completes and unblocks it, or when the duration expires.
-  - `:discard`: the job is discarded (silently finishes). When you choose this option, bear in mind that if a job runs and fails to remove the concurrency lock (or _semaphore_, read below to know more about this), all jobs conflicting with it will be discarded up to the interval defined by `duration` has elapsed. Additionally, Active Jobs that are discarded will have `successfully_enqueued` set to `false`.
+  - `:discard`: the job is discarded. When you choose this option, bear in mind that if a job runs and fails to remove the concurrency lock (or _semaphore_, read below to know more about this), all jobs conflicting with it will be discarded up to the interval defined by `duration` has elapsed.
 
 When a job includes these controls, we'll ensure that, at most, the number of jobs (indicated as `to`) that yield the same `key` will be performed concurrently, and this guarantee will last for `duration` for each job enqueued. Note that there's no guarantee about _the order of execution_, only about jobs being performed at the same time (overlapping).
 
-The concurrency limits use the concept of semaphores when enqueuing, and work as follows: when a job is enqueued, we check if it specifies concurrency controls. If it does, we check the semaphore for the computed concurrency key. If the semaphore is open, we claim it and we set the job as _ready_. Ready means it can be picked up by workers for execution. When the job finishes executing (be it successfully or unsuccessfully, resulting in a failed execution), we signal the semaphore and try to unblock the next job with the same key, if any. Unblocking the next job doesn't mean running that job right away, but moving it from _blocked_ to _ready_. If you're using the `discard` behaviour for `on_conflict`, jobs enqueued while the semaphore is closed will be discarded. Since something can happen that prevents the first job from releasing the semaphore and unblocking the next job (for example, someone pulling a plug in the machine where the worker is running), we have the `duration` as a failsafe. Jobs that have been blocked for more than duration are candidates to be released, but only as many of them as the concurrency rules allow, as each one would need to go through the semaphore dance check. This means that the `duration` is not really about the job that's enqueued or being run, it's about the jobs that are blocked waiting. It's important to note that after one or more candidate jobs are unblocked (either because a job finishes or because `duration` expires and a semaphore is released), the `duration` timer for the still blocked jobs is reset. This happens indirectly via the expiration time of the semaphore, which is updated. In a similar way, when using `discard` as the behaviour to handle conflicts, you might have jobs discarded for up to the `duration` interval if something happens and a running job fails to release the semaphore.
+The concurrency limits use the concept of semaphores when enqueuing, and work as follows: when a job is enqueued, we check if it specifies concurrency controls. If it does, we check the semaphore for the computed concurrency key. If the semaphore is open, we claim it and we set the job as _ready_. Ready means it can be picked up by workers for execution. When the job finishes executing (be it successfully or unsuccessfully, resulting in a failed execution), we signal the semaphore and try to unblock the next job with the same key, if any. Unblocking the next job doesn't mean running that job right away, but moving it from _blocked_ to _ready_. If you're using the `discard` behaviour for `on_conflict`, jobs enqueued while the semaphore is closed will be discarded.
+
+Since something can happen that prevents the first job from releasing the semaphore and unblocking the next job (for example, someone pulling a plug in the machine where the worker is running), we have the `duration` as a failsafe. Jobs that have been blocked for more than `duration` are candidates to be released, but only as many of them as the concurrency rules allow, as each one would need to go through the semaphore dance check. This means that the `duration` is not really about the job that's enqueued or being run, it's about the jobs that are blocked waiting, or about the jobs that would get discarded while the semaphore is closed.
+
+It's important to note that after one or more candidate jobs are unblocked (either because a job finishes or because `duration` expires and a semaphore is released), the `duration` timer for the still blocked jobs is reset. This happens indirectly via the expiration time of the semaphore, which is updated.
+
+When using `discard` as the behaviour to handle conflicts, you might have jobs discarded for up to the `duration` interval if something happens and a running job fails to release the semaphore.
 
 
 For example:
@@ -481,7 +487,7 @@ In this case, if we have a `Box::MovePostingsByContactToDesignatedBoxJob` job en
 
 Note that the `duration` setting depends indirectly on the value for `concurrency_maintenance_interval` that you set for your dispatcher(s), as that'd be the frequency with which blocked jobs are checked and unblocked (at which point, only one job per concurrency key, at most, is unblocked). In general, you should set `duration` in a way that all your jobs would finish well under that duration and think of the concurrency maintenance task as a failsafe in case something goes wrong.
 
-Jobs are unblocked in order of priority but queue order is not taken into account for unblocking jobs. That means that if you have a group of jobs that share a concurrency group but are in different queues, or jobs of the same class that you enqueue in different queues, the queue order you set for a worker is not taken into account when unblocking blocked ones. The reason is that a job that runs unblocks the next one, and the job itself doesn't know about a particular worker's queue order (you could even have different workers with different queue orders), it can only know about priority. Once blocked jobs are unblocked and available for polling, they'll be picked up by a worker following its queue order.
+Jobs are unblocked in order of priority but **queue order is not taken into account for unblocking jobs**. That means that if you have a group of jobs that share a concurrency group but are in different queues, or jobs of the same class that you enqueue in different queues, the queue order you set for a worker is not taken into account when unblocking blocked ones. The reason is that a job that runs unblocks the next one, and the job itself doesn't know about a particular worker's queue order (you could even have different workers with different queue orders), it can only know about priority. Once blocked jobs are unblocked and available for polling, they'll be picked up by a worker following its queue order.
 
 Finally, failed jobs that are automatically or manually retried work in the same way as new jobs that get enqueued: they get in the queue for getting an open semaphore, and whenever they get it, they'll be run. It doesn't matter if they had already gotten an open semaphore in the past.
 
@@ -509,7 +515,9 @@ production:
 Or something similar to that depending on your setup. You can also assign a different queue to a job on the moment of enqueuing so you can decide whether to enqueue a job in the throttled queue or another queue depending on the arguments, or pass a block to `queue_as` as explained [here](https://guides.rubyonrails.org/active_job_basics.html#queues).
 
 
-In addition, mixing concurrency controls with bulk enqueuing (Active Job's `perform_all_later`) is not a good idea because concurrency controlled job needs to be enqueued one by one to ensure concurrency limits are respected, so you lose all the benefits of bulk enqueuing.
+In addition, mixing concurrency controls with **bulk enqueuing** (Active Job's `perform_all_later`) is not a good idea because concurrency controlled job needs to be enqueued one by one to ensure concurrency limits are respected, so you lose all the benefits of bulk enqueuing.
+
+When jobs that have concurrency controls and `on_conflict: :discard` are enqueued in bulk, the ones that fail to be enqueued and are discarded would have `successfully_enqueued` set to `false`. The total count of jobs enqueued returned by `perform_all_later` will exclude these jobs as expected.
 
 ## Failed jobs and retries
 

app/models/solid_queue/job.rb

@@ -10,31 +10,26 @@ class EnqueueError < StandardError; end
 
     class << self
       def enqueue_all(active_jobs)
-        enqueued_jobs_count = 0
+        active_jobs_by_job_id = active_jobs.index_by(&:job_id)
 
         transaction do
           jobs = create_all_from_active_jobs(active_jobs)
-          prepare_all_for_execution(jobs)
-          jobs_by_active_job_id = jobs.index_by(&:active_job_id)
-
-          active_jobs.each do |active_job|
-            job = jobs_by_active_job_id[active_job.job_id]
-
-            active_job.provider_job_id = job&.id
-            active_job.enqueue_error = job&.enqueue_error
-            active_job.successfully_enqueued = job.present? && job.enqueue_error.nil?
-            enqueued_jobs_count += 1 if active_job.successfully_enqueued?
+          prepare_all_for_execution(jobs).tap do |enqueued_jobs|
+            enqueued_jobs.each do |enqueued_job|
+              active_jobs_by_job_id[enqueued_job.active_job_id].provider_job_id = enqueued_job.id
+              active_jobs_by_job_id[enqueued_job.active_job_id].successfully_enqueued = true
+            end
           end
         end
 
-        enqueued_jobs_count
+        active_jobs.count(&:successfully_enqueued?)
       end
 
       def enqueue(active_job, scheduled_at: Time.current)
         active_job.scheduled_at = scheduled_at
 
         create_from_active_job(active_job).tap do |enqueued_job|
-          active_job.provider_job_id = enqueued_job.id
+          active_job.provider_job_id = enqueued_job.id if enqueued_job.persisted?
         end
       end
 

app/models/solid_queue/job/concurrency_controls.rb

@@ -52,7 +52,7 @@ def release_concurrency_lock
 
         def handle_concurrency_conflict
           if concurrency_on_conflict.discard?
-            finished!
+            destroy
           else
             block
           end

app/models/solid_queue/job/executable.rb

@@ -13,8 +13,6 @@ module Executable
 
         after_create :prepare_for_execution
 
-        attr_accessor :enqueue_error
-
         scope :finished, -> { where.not(finished_at: nil) }
       end
 
@@ -39,13 +37,7 @@ def dispatch_all_at_once(jobs)
           end
 
           def dispatch_all_one_by_one(jobs)
-            jobs.each do |job|
-              begin
-                job.dispatch
-              rescue EnqueueError => e
-                job.enqueue_error = e
-              end
-            end
+            jobs.each(&:dispatch)
           end
 
           def successfully_dispatched(jobs)

test/integration/concurrency_controls_test.rb

@@ -197,11 +197,10 @@ class ConcurrencyControlsTest < ActiveSupport::TestCase
   end
 
   test "discard jobs when concurrency limit is reached with on_conflict: :discard" do
-    # Enqueue first job - should be executed
-    job1 = DiscardableUpdateResultJob.perform_later(@result, name: "1", pause: 0.2)
-    # Enqueue second job - should be discarded due to concurrency limit
+    job1 = DiscardableUpdateResultJob.perform_later(@result, name: "1", pause: 3)
+    # should be discarded due to concurrency limit
     job2 = DiscardableUpdateResultJob.perform_later(@result, name: "2")
-    # Enqueue third job - should also be discarded
+    # should also be discarded
     job3 = DiscardableUpdateResultJob.perform_later(@result, name: "3")
 
     wait_for_jobs_to_finish_for(5.seconds)
@@ -212,12 +211,11 @@ class ConcurrencyControlsTest < ActiveSupport::TestCase
 
     # All jobs have finished and have no blocked executions
     jobs = SolidQueue::Job.where(active_job_id: [ job1, job2, job3 ].map(&:job_id))
-    assert_equal 3, jobs.count
+    assert_equal 1, jobs.count
 
-    jobs.each do |job|
-      assert job.finished?
-      assert_nil job.blocked_execution
-    end
+    assert_equal job1.provider_job_id, jobs.first.id
+    assert_nil job2.provider_job_id
+    assert_nil job3.provider_job_id
   end
 
   test "discard on conflict across different concurrency keys" do

test/models/solid_queue/job_test.rb

@@ -176,11 +176,10 @@ class DiscardableNonOverlappingGroupedJob2 < NonOverlappingJob
 
     travel_to 10.minutes.from_now
 
-    count = SolidQueue::ScheduledExecution.dispatch_next_batch(10)
-
-    assert_not scheduled_job.reload.scheduled?
-    assert_not scheduled_job.ready?
-    assert scheduled_job.finished?
+    # The scheduled job is not enqueued because it conflicts with
+    # the first one and is discarded
+    assert_equal 0, SolidQueue::ScheduledExecution.dispatch_next_batch(10)
+    assert_nil SolidQueue::Job.find_by(id: scheduled_job.id)
   end
 
   test "enqueue jobs in bulk" do
@@ -212,16 +211,20 @@ class DiscardableNonOverlappingGroupedJob2 < NonOverlappingJob
       NonOverlappingJob.new(@result),
       AddToBufferJob.new(6).set(wait: 2.minutes),
       NonOverlappingJob.new(@result),
-      DiscardableNonOverlappingJob.new(@result)
+      DiscardableNonOverlappingJob.new(@result) # this one won't be enqueued
     ]
+    not_enqueued = active_jobs.last
 
-    assert_job_counts(ready: 3, scheduled: 1, blocked: 1, discarded: 1) do
+    assert_job_counts(ready: 3, scheduled: 1, blocked: 1) do
       ActiveJob.perform_all_later(active_jobs)
     end
 
-    jobs = SolidQueue::Job.last(6)
-    assert_equal active_jobs.map(&:provider_job_id).sort, jobs.pluck(:id).sort
-    assert active_jobs.all?(&:successfully_enqueued?)
+    jobs = SolidQueue::Job.last(5)
+    assert_equal active_jobs.without(not_enqueued).map(&:provider_job_id).sort, jobs.pluck(:id).sort
+    assert active_jobs.without(not_enqueued).all?(&:successfully_enqueued?)
+
+    assert_nil not_enqueued.provider_job_id
+    assert_not not_enqueued.successfully_enqueued?
   end
 
   test "discard ready job" do
@@ -334,13 +337,6 @@ class DiscardableNonOverlappingGroupedJob2 < NonOverlappingJob
     assert_raises SolidQueue::Job::EnqueueError do
       AddToBufferJob.perform_later(1)
     end
-
-    # #perform_later with a block doesn't raise ActiveJob::EnqueueError,
-    # and instead set's successfully_enqueued? to false
-    assert_not AddToBufferJob.perform_later(1) do |active_job|
-      assert_not active_job.successfully_enqueued?
-      assert_equal SolidQueue::Job::EnqueueError, active_job.enqueue_error.class
-    end
   end
 
   test "enqueue successfully inside a rolled-back transaction in the app DB" do
@@ -376,13 +372,8 @@ def assert_blocked(&block)
       assert SolidQueue::Job.last.blocked?
     end
 
-    def assert_discarded(&block)
-      assert_job_counts(discarded: 1, &block)
-      assert SolidQueue::Job.last.finished?
-    end
-
-    def assert_job_counts(ready: 0, scheduled: 0, blocked: 0, discarded: 0, &block)
-      assert_difference -> { SolidQueue::Job.count }, +(ready + scheduled + blocked + discarded) do
+    def assert_job_counts(ready: 0, scheduled: 0, blocked: 0, &block)
+      assert_difference -> { SolidQueue::Job.count }, +(ready + scheduled + blocked) do
         assert_difference -> { SolidQueue::ReadyExecution.count }, +ready do
           assert_difference -> { SolidQueue::ScheduledExecution.count }, +scheduled do
             assert_difference -> { SolidQueue::BlockedExecution.count }, +blocked, &block