Skip to content

Reject connections with no transactions open in specs

Thong Kuah requested to merge reject_connections_with_no_transactions_open into master

What does this MR do and why?

Reject connections with no transactions open in specs

Due to a race condition, it is possible for another thread (e.g. ActionCable) to create a new DB connection. This new DB connection will not have access to the transaction opened from the before_all_adapter.

To workaround this issue, we prevent DB connections that has no access to the before_all_adapter transaction from being checked in. So that this bad DB connection can't be used by tests by accident.

The race condition goes something like this:

  1. Transactional test starts
  2. before_all_adapter begins a DB transaction for each connection pool. We need this for before_all and let_it_be to work with transactional tests.
  3. Code checks out DB connections as usual. These DB connections are re-used across threads using lock_thread, and thread connection cache.
  4. A test finishes. DB connection is checked back in. This means the thread connection cache is empty. The checked in DB connection is now available.
  5. At this time, for the next test, the test thread also requests a DB connection to be checked out. It reuses the available connection.
  6. At the same time, another thread (ActionCable) requests a DB connection to be checked out. Because the thread connection cache is empty, and all available connections are taken, Rails has to create a new DB connection. This DB connection has no access to the before_all_adapter transaction.
  7. Around this time, the test thread populates the thread connection cache with the checked out DB connection.
  8. Tests continue to run.
  9. Another test finishes. DB connection is checked back in again.
  10. At the same time, the ActionCable execution completes. It starts checking back in DB connections, including the DB connection that has no access to the before_all_adapter transaction
  11. Another test starts requesting a DB connection. The thread connection cache is empty, so we look in available DB connections. The first available DB connection is the bad one.
  12. Tests now fail with symptoms like:
    Couldn't find User with 'id'=337

Related issue: #416663 (closed)

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

  1. Reduce statement_timeout in config/database.yml to 50s
  2. export GITLAB_TEST_EAGER_LOAD=1
  3. Run bundle exec rspec -f d spec/features/projects/work_items/work_item_spec.rb --seed 49620

Without this patch, you may see sometimes failures like:

Couldn't find User with 'id'=337

With this patch, you will sometimes see, but the specs pass:

Removing #<ActiveRecord::ConnectionAdapters::PostgreSQLAdapter:0x0000000129a41288> from pool as has no open transactions

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Thong Kuah

Merge request reports