Healthy DB connections are thrown away after transaction serialization failure

[eric-christian]

Hello :)

Last week we updated from 6.1.3.2 to 6.1.4.1 and noticed that there are many connections dropped and reestablished now.
We also got ActiveRecord::ConnectionNotEstablished within nested transactions in some special cases. (Explained in more detail at the bottom)

After some digging, i noticed that connections are thrown away after an ActiveRecord::TransactionRollbackError is raised. Which is the parent class for ActiveRecord::SerializationFailure and ActiveRecord::Deadlocked.
At least for the ActiveRecord::SerializationFailure exception, this behavior is giving us some trouble.

We are using the transaction isolation level "serializable", where SerializationFailure's are a common thing.

We are currently using CockroachDB but as far as i know neither mysql nor postgresql leaves the connection in an undefined state after a SerializationFailure. Simply rollback and retry the transaction on the same connection is a valid way to go. So dropping the connection is not necessary. If there is a case with a DB where dropping the connection is actually required, can we maybe only drop the connection in that case?

v6.1.3.2...v6.1.4#diff-353f4f101b1bb04938d761b6e29c88e0f147084e9e276854a77ae5d9a9a3b2a8R3
v6.1.3.2...v6.1.4#diff-dd0ea9dfea1fd486f2b1b5eac8c798f839467173a1c2315d3ad04f4098f7dda3R333

I don't know if this was meant to be a performance improvement or actually a real issue with some databases that i don't know of. But skipping a ROLLBACK and dropping a connection are two different things. I think, but i'm not 100% sure about it right now, that the ROLLBACK [SAVEPOINT ...] is still required after a SerializationFailure.
I think you have to signal the DB that you want to ROLLBACK the failed transaction if you attempt to retry it on the same connection. This probably depends on the DB? If there is different behavior, maybe leave it to the adapter to decide what way to go? So far i don't understand why this changes were made.

Example time! :)
We are currently running two different implementations of retryable transactions.

1. A basic one with simple exponential backoff, which still works but issues unnecessary reconnects.

  # simplified code
  simple_retryable_transaction do
    return yield if within_transaction? # no retries for nested transactions

    loop do
      break ActiveRecord::Base.connection.transaction { yield } # RealTransaction
    rescue ActiveRecord::SerializationFailure => error      
      #  update metrics / APM to see contention
      raise unless more_retries?
      sleep exponential_backoff_seconds
   end
 end

2. A more specialised and faster one, where a savepoint is rolled back. Probably only featured by CockroachDB? This one is now broken. After the first SerializationFailure the Inner- and Outer-Transaction are not on the same connection anymore.

  # simplified code
  advanced_retryable_transaction do
    return yield if within_transaction? 

    ActiveRecord::Base.connection.transaction(joinable: false) do
      loop do
       # on the second iteration, this will create a RealTransaction on a new connection.
        break ActiveRecord::Base.connection.transaction(requires_new: true) { yield }
      rescue ActiveRecord::SerializationFailure => error      
        #  update metrics / APM to see contention
        # since the DB is prioritising the session with the most retries during contention evaluation:
        #   no sleep and no limited amount of retries
     end
     # If we had a SerializationFailure during the loop which we solved by retrying,
     #   this transaction is now attempting to issue a COMMIT on a closed connection \o/
   end
end

If required i'll try to put together some examples with https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#create-an-executable-test-case

Best regards,
Eric

Expected behavior

Keep the connection after ActiveRecord::SerializationFailure and probably even after ActiveRecord::Deadlocked.

Actual behavior

Connections are thrown away after ActiveRecord::SerializationFailure and ActiveRecord::Deadlocked.

System configuration

Rails version: 6.1.4.1

Ruby version: 2.7.3

[ghiculescu]

v6.1.3.2...v6.1.4diff-353f4f101b1bb04938d761b6e29c88e0f147084e9e276854a77ae5d9a9a3b2a8R3
v6.1.3.2...v6.1.4diff-dd0ea9dfea1fd486f2b1b5eac8c798f839467173a1c2315d3ad04f4098f7dda3R333

These links aren't working for me, are you sure they're the right link?

[eric-christian]

Ah sorry, i simply copy&pasted them without testing them!
I wanted to point out the changes within the comparison-view-thingy but somehow github won't let me do that.

Here are the links to the lines that i initially tried to link to:

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Line 324 in 18707ab

transaction.state.invalidate! if error.is_a? ActiveRecord::TransactionRollbackError

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Line 335 in 18707ab

@connection.throw_away! unless transaction.state.rolledback?

Best regards,
Eric

[ghiculescu]

Hmmmm, does the change in #40541 look like the cause of your issues?

[eric-christian]

Yes, looks like it.

Is there a missunderstanding between a failed rollback and a failed transaction?
I'm fine with dropping the connection if the rollback fails or if something raises during the attempted rollback, because that would leave the session in an undefined state.
But not when a transaction fails with a serialisation failure, that is legit behavior with a defined state.
Am i missing something?

Best regards,
Eric

[ghiculescu]

@caueguerra or @jahfer, any thoughts on this one?

[eric-christian]

Any updates on this?

[jahfer]

As you already found, we throw away the connection if it hasn't been rolled back successfully:

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Line 335 in 18707ab

@connection.throw_away! unless transaction.state.rolledback?

Forgive my naive knowledge of ActiveRecord::SerializationFailure, but I would have assumed transaction.state.rolledback? would be true, given the call to #rollback_transaction in the transaction rescue:

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Lines 322 to 327 in 5d9ee65

	rescue Exception => error
	if transaction
	transaction.state.invalidate! if error.is_a? ActiveRecord::TransactionRollbackError
	rollback_transaction
	after_failure_actions(transaction, error)
	end

What we were seeing in #40541 was that a new exception inside of the rollback left the transaction open and left the connection in the pool. Do you know what state the transaction has when you enter the ensure block of #within_new_transaction for a SerializationFailure? Is it possible #rollback_transaction is raising its own exception in your example code?

[eric-christian]

Hi @jahfer,

Thank you for your feedback.

#rollback_transaction does not raise any exceptions within my example code. It successfully issues a ROLLBACK statement to the database.
And in that case, transaction.state.rolledback? is true within the ensure block. At least that was the case in the past.

From my understanding, transaction.state.rolledback? will only be true if transaction.rollback has been called at some point.
But that is not the case because there is this line transaction.rollback unless transaction.state.invalidated? here

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Lines 308 to 314 in 5d9ee65

	def rollback_transaction(transaction = nil)
	@connection.lock.synchronize do
	transaction ||= @stack.pop
	transaction.rollback unless transaction.state.invalidated?
	transaction.rollback_records
	end
	end

I think that in the situation where an exception is actually raised within a rollback, it is totally valid to drop the connection because something went bonkers. Because a rollback should always be possible as long as there is a transaction. At least that is what i know about databases and transactions. But again, ActiveRecord::TransactionRollbackError is not raised when a ROLLBACK fails, but when a transaction is required to ROLLBACK and tried again.

What you found within #40541 really seems to have been a problem with the error handling within #within_new_transaction or more exactly with the ensure block not taking into account that the rescue block might have been unable to successfully issue a ROLLBACK statement. And i think the changes to the ensure block are legit. While not 100%. There are still possibilities that the ROLLBACK at this line

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Line 338 in 5d9ee65

rollback_transaction

or this line

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Line 353 in 5d9ee65

rollback_transaction(transaction) unless transaction.state.completed?

within the ensure block fails without the connection being dropped.

Still, to my understanding this entire line

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Line 324 in 5d9ee65

transaction.state.invalidate! if error.is_a? ActiveRecord::TransactionRollbackError

and the unless .... part at this line

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Line 311 in 5d9ee65

transaction.rollback unless transaction.state.invalidated?

are not necessary to fix the problem in #40541. Maybe the entire transaction.state.invalidated state is not necessary.

To my understanding ActiveRecord::TransactionRollbackError was never part of the problem nor should be part of the solution to #40541 . So i wonder why it showed up here. Just because of the name? Or was there a situation where it was encountered and led to trouble with the ROLLBACK? Maybe i am missing something? Maybe a special behavior of a database that i don't know of, where a ROLLBACK after a serialisation error is invalid? But that doesn't seem to match with the description of the problem in #40541 where a commit was actually possible later on. 🤷‍♂️

Best regards,
Eric

[jahfer]

Apologies for the delay. This took a little bit of tracing down, but it looks like the issue you're describing was introduced in 57053ad (cc @rafaelfranca).

The line added in #40541 that throws away the connection:

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Line 335 in 18707ab

@connection.throw_away! unless transaction.state.rolledback?

…should actually avoid doing so if transaction.state.rolledback? or transaction.state.invalidated?. This is because invalidated transactions are already rolled back at the DB level, so there's no risk of accidentally committing them (at least this is my very poor understanding of ActiveRecord::SerializationFailure and ActiveRecord::Deadlocked).

The invalidated state was introduced in #30922 and merged after #40541.

Does my description make sense to you as well, @eric-christian?

[eric-christian]

Hello @jahfer,

it makes sense to me, but there is one more line to look at.

rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

Line 311 in 5d9ee65

transaction.rollback unless transaction.state.invalidated?

This is a topic i can't easily explain from memory. I have to play, test and dig around again every time. And of course databases tend to act differently. 🤷

MySQL
After a deadlock error MySQL does not need the ROLLBACK at line 311, but MySQL also does not care if you throw an extra ROLLBACK at it.
But a ROLLBACK TO SAVEPOINT abc; will fail with something like unknown savepoint after a deadlock error. Since the entire transaction, including all savepoints, is rolled back already. So in that case, if we are to rollback a savepoint transaction in line 311, it would be valid to be skipped. Maybe the skipping should happen within the SavepointTransaction instance.

PostgreSQL
After provoking a serialisation error within postgres, for every future statement it returns an error ERROR: current transaction is aborted, commands ignored until end of transaction block until the transaction block is explicitly closed. A ROLLBACK is a valid statement to close the transaction block. So we should not skip the rollback at line 311. With the current changes, i don't think it will work with postgres.
I ran out of time to go into details with savepoints and postgres, but would assume that in that case it acts similar to MySQL. (Famous last words 😄)

I need to do more testing before i could really elaborate on this. Maybe i find some time for it tomorrow.

Best regards
Eric