Concurrency controlled jobs cause infinite loops when perform_limit is exceeded in test environments

[pgvsalamander]

If a background job created using perform_later exceeds the concurrency limit in the test environment it will get stuck in a loop:

1. perform_later is called
2. The job is executed immediately because it's running in a test environment (execution_mode = :inline)
3. The job throws ConcurrencyExceededError
4. The job is re-queued with a delay
5. goto 2

This is tangentially related to #591, which was initially addressed in #593 by having the job skip concurrency limits if perform_now is used. I would expect perform_later/retry_job to ignore the limit the same way when executing inline.

Example code:

class InfiniteLoopTestJob < ActiveJob::Base
	include GoodJob::ActiveJobExtensions::Concurrency

	queue_as :bug_repro_job

	good_job_control_concurrency_with(
		total_limit: 2,
		perform_limit: 1,
		key: "key"
	)

	def perform(*args)
		ap "jobs to queue: #{$jobs_to_queue}"
		$jobs_to_queue -= 1
		return if $jobs_to_queue < 0

		InfiniteLoopTestJob.perform_later
	end
end

(Run this task with RAILS_ENV=test)

namespace :infinite_loop_test do
	task :run => :environment do
		$jobs_to_queue = 5

		ap "starting task"

		InfiniteLoopTestJob.perform_later
	end
end

Output:

"starting task"
"jobs to queue: 5"
rails aborted!
SystemStackError: stack level too deep
/home/user/git_repos/repo/app/jobs/infinite_loop_test_job.rb:17:in `perform'
/home/user/git_repos/repo/lib/tasks/infinite_loop_test.rake:9:in `block (2 levels) in <main>'
/home/user/git_repos/repo/bin/rails:5:in `<top (required)>'
/home/user/git_repos/repo/bin/spring:10:in `block in <top (required)>'
/home/user/git_repos/repo/bin/spring:7:in `tap'
/home/user/git_repos/repo/bin/spring:7:in `<top (required)>'
Tasks: TOP => infinite_loop_test:run
(See full trace by running task with --trace)

[bensheldon]

@pgvsalamander oops! You're totally right. Looks like there should be another guard here that bails if the adapter is an inline one:

good_job/lib/good_job/active_job_extensions/concurrency.rb

Lines 17 to 21 in c8f19ee

	# Don't attempt to enforce concurrency limits with other queue adapters.
	next(block.call) unless job.class.queue_adapter.is_a?(GoodJob::Adapter)
	# Always allow jobs to be retried because the current job's execution will complete momentarily
	next(block.call) if CurrentThread.active_job_id == job.job_id

Is that a PR you'd be able to make?

[bensheldon]

I made a PR to ignore perform_limit when the adapter is inline: #610

This does make me wonder whether the Inline adapter should have different behavior. The inline adapter's use case is in the test environment. I'm thinking that Inline should execute inline once and then either:

• The exception is raised (e.g. it ignores retry handlers)
• The job is re-enqueued via the retry-handler, but is treated as if the execution mode is :external e.g. the job is stored to execute in the future, but it won't be executed by the inline adapter.

I'll make another issue to discuss.

[pgvsalamander]

The exception is raised (e.g. it ignores retry handlers)

I don't think it should always raise an exception. In my case a job causing another instance of itself to be queued is the desired behavior, and I imagine it may be for others as well.

I can't get into too may details but for my case I have a job A that maintains some preprocessed calculations for items in the database, and depending on the state of the calculations queues job B to do additional work on more items. This eventually flows back to queueing job A to update the first item again, which does not queue job B this time as the calculations would be fully up to date at this point. This works great when running in the async environment as it allows me to insert updated data at any point in the workflow and the jobs will run and queue additional jobs as necessary, always returning to a deterministic state.

The job is re-enqueued via the retry-handler, but is treated as if the execution mode is :external e.g. the job is stored to execute in the future, but it won't be executed by the inline adapter.

Perhaps there could be some method to run all the jobs in the queue inline at that point? That would allow better control over where/when the jobs in the test execute.

An alternative option is that if you're currently executing a job, you could send any further jobs to the queue and then run them once the current job exits. This would ensure that two jobs don't overlap while still allowing jobs to run immediately within tests.

I have also encountered some issues where the job never runs because there are jobs from previous tests still in the DB causing it to exceed the total_limit. It's not really a bug, but it may be relevant to the changes made because of this discussion.

[bensheldon]

@pgvsalamander sorry, I've slept on this one too many nights. I think I'm leaning now toward your suggestion:

if you're currently executing a job, you could send any further jobs to the queue and then run them once the current job exits. This would ensure that two jobs don't overlap while still allowing jobs to run immediately within tests.

I think that would resolve the stack-level-too-deep problem. Here's my thoughts:

• When in an inline adapter, if you enqueue inside of an already executing job, the job is put into the external queue.
• At the end of the outermost inline enqueing/execution, the inline adapter will then loop over and execute all of the jobs in the external queue
• Have a callable/public adapter method like ApplicationJob.queue_adapter.execute_inline that does the looping over and executing of all of the jobs on the external queue.
• [breaking change?] subsequently, I think inline should only execute jobs that are immediate/past-due-scheduled. That would ensure that:
  • Instead of stack-level-too-deep, infinite retries with backoff does not result in an infinite loop [similarly bad to current behavior, maybe even worse because it will hang rather than blow up]
  • You could use timecop/time-travel in your tests to execute scheduled jobs (e.g. travel_to(5.minutes.from_now) { ApplicationJob.queue_adapter.execute_inline }) and have better behavior than currently.

[pgvsalamander]

Executing all the jobs immediately regardless of the time they are scheduled would help my use case. Sometimes when creating jobs I will set them to run 10s into the future, to make sure that whatever changes triggered the job's creation had time to be saved to the DB. It would be nice to be able to know that all the jobs had settled without having to use travel_to and manually triggering the jobs to run. An option to the ApplicationJob.queue_adapter.execute_inline method that would run all pending jobs regardless of scheduled jobs could be a good compromise.

I also see the value in only running the jobs that are past-due, that would make it easier to test the interaction of multiple jobs with each other/the main thread.

They both have their use cases, so I propose creating a new execution mode - :inline would queue the jobs and execute them as soon as the previous job ends, and the new execution mode would only run the jobs that are past due (:inline_scheduled maybe? I'm terrible with names). That would avoid the breaking change to how the :inline execution mode works, while making the new feature trivial to switch to for anyone who wants to use it.

[bensheldon]

@pgvsalamander thank you for all of the feedback and suggestions. I just put together a pull request that I think addresses your needs, and is a good improvement: #615

[pgvsalamander]

No, thank you for the rapid implementation! GoodJob is quickly becoming a core component of our project and I don't know how we'd get anything done without it.

[bensheldon]

@pgvsalamander I just released v2.16.0 that includes #615

[pgvsalamander]

I'm not sure how you prefer to handle issue tickets, are you expecting me to close this issue now that the update is released or would you like it left open for future discussion?

[bensheldon]

@pgvsalamander no sweat. I can close this out.