ActiveRecord::QueryCache using the new Rails executor may result in dirty query caches across threads. Releasing a connection early in one thread may return it to the pool with cache still enabled and dirty, and the executor loses track of the connection so may clear another, and another thread may pick up the dirty cache and never turn it off.

Steps to reproduce

In practise this has occurred to us in a high-concurrency production environment across threads. We can reliably reproduce it with explicitly scheduled threads:

class Example < ActiveRecord::Base
  connection.execute <<~SQL
    CREATE TABLE examples (id int primary key);
  SQL
end

Example.create!(id: 1)

ActiveRecord::Base.connection_pool.connections.none?(&:query_cache_enabled)
# => true

thread_1_connection = nil
thread_1 = Thread.new do
  Rails.application.executor.wrap do
    thread_1_connection = ActiveRecord::Base.connection
    task = Example.find(1)
    thread_1_connection.query_cache.empty?
    # => false
    ActiveRecord::Base.clear_active_connections!
    # Sleep so thread 2 can pick up this connection
    sleep
    # The executor will grab a different connection and clear its query cache when returning from this block
  end
end

thread_2_connection = nil
thread_2 = Thread.new do
  # The executor grabs thread 1's released connection, and stores that the connection has query cache turned on by default
  Rails.application.executor.wrap do
    thread_2_connection =  ActiveRecord::Base.connection
    thread_2_connection == thread_1_connection
    # => true
    thread_2_connection.query_cache.empty?
    # => false
    sleep
    # The executor does clear the cache but doesn't change query_cache_enabled when returning here
  end
end

thread_1.wakeup
thread_1.join
thread_1_connection.query_cache_enabled
# => true

thread_2.wakeup
thread_2.join
ActiveRecord::Base.connection_pool.connections.none?(&:query_cache_enabled)
# => false

ActiveRecord::Base.connection.execute "DROP TABLE examples"

Expected behaviour

ActiveRecord connections returned to the connection pool should have their query caches cleared and turned off (or back to the default).

Actual behaviour

Threads may receive connections with dirty query caches and some connections may end up with query caching permanently enabled.

System configuration

Rails version: 5.0.0.1 (master is the same)
Ruby version: 2.3.1

First solution attempt

Store the connection in the executor state:

module ActiveRecord
  class QueryCache
    # ...
    def self.run
      connection    = ActiveRecord::Base.connection
      enabled       = connection.query_cache_enabled
      connection.enable_query_cache!

      [connection, enabled]
    end

    def self.complete((connection, enabled))
      connection.clear_query_cache
      connection.disable_query_cache! unless enabled
    end
    # ...
  end
end

There is still an edge case here: if the connection is returned to the pool early there is still a window where another thread may checkout the connection and see its dirty cache, and potentially store the cache is on by default, again landing us in a connection with query cache stuck enabled.

If an executor is responsible for checking out the AR connection then it's probably reasonable to assume it's also responsible for checking it back in. In practise, however, it's fairly easy to accidentally break this assumption.

A Better Solution?

Perhaps instead query caching should be enabled/cleared by the connection pool when checking connections in/out? It would prevent touching connections until they are needed. It would allow query caching in more complex deployments using multiple connection specifications. It would also be nice to be able to configure whether query caching is used at all at a connection specification level.

/cc @matthewd