Using ActiveJob async adapter (and threads in general) with before_all is risky

[aburgel]

Tell us about your environment

Ruby Version: 3.2.2

Framework Version (RSpec, Minitest, FactoryGirl, Rails, whatever): RSpec 3.12, Rails 7.0.8

TestProf Version: 1.2.3

The docs could use a caveat about the dangers of using before_all with threading, in particular, using ActiveJob's async queue adapter.

The case I ran into was using before_all for system specs with the async queue adapter. While the test is running, things work well, but it's possible for a job to get executed after the test completes which can mess up the db connections and cause hard to debug failures.

During teardown, the use_transactional_fixtures functionality will unset the lock_thread, so if after that point, an async job runs, it may grab the connection with the open transaction. If the next test starts before the job completes, a new connection will be used which means a fresh transaction without any fixture data.

There's a comment that implies that the async adapter case is handled, but it doesn't account for Rails unsetting lock_thread in teardown.

Some ideas on how to improve this situation:

1. Is it possible to keep a reference to the original connection and warn or raise in test setup if a different connection is active?
2. Can we add a caveat to the docs describing the risks of using async adapter and threads? Or maybe with async adapter in particular there is a safe way to set it up? Like forcing all the jobs to complete before the next test runs?

[palkan]

There's a comment that implies that the async adapter case is handled, but it doesn't account for Rails unsetting lock_thread in teardown.

Yeah, the hack was added to support using async execution triggered right in the before_all block.

Is it possible to keep a reference to the original connection and warn or raise in test setup if a different connection is active?

I think, the possible solution could be is to override lock_thread= and prevent it from being set to false within before_all. Still, if something is being executed at the end of the test example (and outside of before_all), there can be the same problems 🤷‍♂️

The same problem occurs if you're not using before_all but regular transactional tests; so, it's more about ensuring that background tasks are executed before teardown.

One option is to add a global after hook to wait for in-flights tasks:

config.after(:each) do
  if ActiveJob::QueueAdapters::AsyncAdapter === ActiveJob::Base.queue_adapter
    # shutdown previous instance, waits for in-flight tasks by default
    ActiveJob::Base.queue_adapter.shutdown
    # create a new one
    ActiveJob::Base.queue_adapter = :async
  end
end

Can we add a caveat to the docs describing the risks of using async adapter and threads?

Yeah, that would helpful

[aburgel]

The same problem occurs if you're not using before_all but regular transactional tests; so, it's more about ensuring that background tasks are executed before teardown.

Sure, but it wouldn't be noticeable since you wouldn't expect database state to carry over, so an async job "stealing" the last connection wouldn't matter much.

One option is to add a global after hook to wait for in-flights tasks

I tried this but it didn't work reliably. I think the issue is that the teardown that unsets lock_thread will run before a global after(:each). The problem is slightly improved in that all jobs should be complete before the next example runs, but if multiple jobs run, then it's possible that multiple threads are running concurrently with multiple connections. Then when the next example runs, an arbitrary connection will get "locked", not necessarily the one with the open transaction. You could re-enable lock_thread in this hook, but that may not catch executions that occur in the gap between the example ending and this hook starting.

Perhaps there's some way to force this job clearing hook to run first, before the lock_thread is unset, but I'm reaching the limits of my rspec knowledge here.

[palkan]

I think the issue is that the teardown that unsets lock_thread will run before a global after(:each).

It shouldn't. It's invoked by Rails in the after_teardown hook: https://github.com/rails/rails/blob/253cddd92d7dc7ed170691c173d079073efdc3c2/activerecord/lib/active_record/test_fixtures.rb#L16 (which I assume to be invoked after all after callbacks).

However, it seems that there was a problem in rspec-rails (fixed recently): rspec/rspec-rails#2596

[aburgel]

I think the issue is that hooks defined at the suite level run after hooks defined in each example or context. It looks like rspec mixes in a module that invokes after_teardown, which (I think) is considered at the context level, so it will run first.

Maybe the answer here is to include a module that defines an after hook. But I'd say the tl;dr of all this is that it's complicated and probably deserves it's own caveat or some actual code that others can use directly for this scenario.

I'd be happy to help out if you let me know which direction is best for this project.

[palkan]

It looks like rspec mixes in a module that invokes after_teardown, which (I think) is considered at the context level, so it will run first.

Yeah, seems so.

I wonder if the problem is before_all-specific, and, thus, should be fixed/mentioned in this project. As far as I understand, the same problem may occur with plain before or before(:all) hooks.

Looks the most robust way of dealing with it is to patch the #teardown_fixtures method to inject custom cleanup logic:

ActiveRecord::TestFixtures.prepend(Module.new do
  def teardown_fixtures
    # do some cleanup
    super
  end
end)

Note that this only would work in recent Ruby versions (prepending only included module has no effect in older versions)