Executing perform_now on a good_job with GoodJobs::ActiveJobExtensions::Concurrency can run twice

[gap777]

Setup:

• Good job with concurrency controls:

class TestJob < ApplicationJob
    include GoodJob::ActiveJobExtensions::Concurrency

    good_job_control_concurrency_with(
      total_limit: 1, # Maximum number of unfinished jobs to allow with the concurrency key
      key: -> do
        args = arguments.first
        foo = args[:foo]
        "#{self.class.name}_#{foo}
      end
    )

• code which executes said job via perform_now:

     def api_endpoint
          TestJob.perform_now(foo: :bar)
     end

• call said endpoint from within another job:

class BackgroundJob < ApplicationJob
end

• run background job in the background:

BackgroundJob.perform_later

Expected Output:

TestJob runs once

Observed Output

TestJob runs twice, and the concurrency violation is observed:

15:10:46 web.1  | [ActiveJob] [BackgroundJob] [f2f91663-d772-4d10-b120-9d34e7a95739] [TestJob] [c447ec3a-c461-4756-9ac3-065f8564ee7e] Performing TestJob (Job ID: c447ec3a-c461-4756-9ac3-065f8564ee7e) from GoodJob(default) with arguments: {:foo=>:bar}

15:10:46 web.1  | [ActiveJob] [BackgroundJob] [f2f91663-d772-4d10-b120-9d34e7a95739] [TestJob] [c447ec3a-c461-4756-9ac3-065f8564ee7e] Enqueued TestJob (Job ID: c447ec3a-c461-4756-9ac3-065f8564ee7e) to GoodJob(default) at 2024-04-23 19:10:50 UTC with arguments: {:foo=>:bar}

15:10:46 web.1  | [ActiveJob] [BackgroundJob] [f2f91663-d772-4d10-b120-9d34e7a95739] [TestJob] [c447ec3a-c461-4756-9ac3-065f8564ee7e] Retrying TestJob (Job ID: c447ec3a-c461-4756-9ac3-065f8564ee7e) after 1 attempts in 3 seconds, due to a GoodJob::ActiveJobExtensions::Concurrency::ConcurrencyExceededError (GoodJob::ActiveJobExtensions::Concurrency::ConcurrencyExceededError).

### Suspected Culprit

good_job-3.27.3/lib/good_job/active_job_extensions/concurrency.rb, L78:

    if CurrentThread.execution.blank?
        logger.debug("Ignoring concurrency limits because the job is executed with `perform_now`.")
        next
      end

[bensheldon]

This is intended or more specifically, there's nothing GoodJob can do about it. perform_now bypasses the Active Job Adapter and directly invokes the job's perform (that's how Active Job works). GoodJob doesn't have a job record against which to lock for controlling concurrency.

[gap777]

@bensheldon I realize you may not be able to detect when you are in the foreground, and when you are in the background. I'm not suggesting GoodJob should do any concurrency logic in that scenario; I'm just suggesting that something in GoodJob's concurrency mixin is in error, since the Job is executed twice. Removing the concurrency mixin does prevent the unexpected behavior of double execution.

[bensheldon]

oh! sorry I misunderstood. I think you're saying it's something like this:

class BackgroundJob < ApplicationJob
  def perform
    TestJob.perform_now
  end
end

# and then...
BackgroundJob.perform_later

So that would imply that we need this change because the context of the BackgroundJob is bleeding into the TestJob#perform_now. So we need:

- if CurrentThread.execution.blank?
+ if CurrentThread.execution.blank? || CurrentThread.execution.active_job_id != job_id
  logger.debug("Ignoring concurrency limits because the job is executed with `perform_now`.")
  next
end

[gap777]

I think you better understand what I was getting at... the context is bleeding through.

Is the fix that simple?

[bensheldon]

Yep, I think it's that simple 😊 I made a PR: #1336

[bensheldon]

I just released (what I hope is) the fix in https://github.com/bensheldon/good_job/releases/tag/v3.28.1

[gap777]

Perfect! Thank you!