Records created outside of transaction block gets rolled back when transaction raises exception

[sato11]

What Ruby, Rails and RSpec versions are you using?

• Ruby version: 3.1.2
• Rails version: 7.0.2.3
• RSpec versions:
  • rspec-core 3.11.0
  • rspec-expectations 3.11.0
  • rspec-mocks 3.11.1
  • rspec-rails 5.0.3
  • rspec-support 3.11.0

Observed behaviour

When the application code raises exception in .transaction, records created outside of its block, including test data, got rolled back.

Expected behaviour

Raising exception in .transaction block rolls back only the data created in the .transaction.

Background

Although I'm attaching the reproduction repo below, I bet it doesn't look very straightforward. So let me explain why here.

I came across this behavior while I was struggling with a method which looked like this (but far larger): sato11/rspec-rails-transaction-sample@169a98a. I wanted it to become this: sato11/rspec-rails-transaction-sample@5f8d90b. Because on detecting deadlock our innoDB rolls back the transaction (doc), we needed to not only rescue it but also explicitly BEGIN again. However when I wrapped .transaction in begin-end the test started to fail and that's what I'm reporting here.

I'm not really sure if it's somehow related to #use_transactional_fixtures, but I bet it's a very subtle one. I'd be glad if I could get any advice of how I can work on this or how I can write the test in a different way 🙏

Can you provide an example app?

Sure.
https://github.com/sato11/rspec-rails-transaction-sample

[JonRowe]

I think this is the default behaviour with config.use_transactional_fixtures = true as you are creating nested transactions when using this, which my reading of / memory for mysql (I don't use it anymore myself) is not as well supported as Postgres. Can you turn this setting to false and see if that solves the issue for you? If so you'll need to raise this upstream with Rails as we don't implement that inside RSpec, we just turn the rails setting on/off for you.

[pirj]

Nested transactions are very tricky, especially with their (subjectively) nonsensical defaults.

By default, this makes all database statements in the nested transaction block become part of the parent transaction.

if :requires_new is set, the block will be wrapped in a database savepoint acting as a sub-transaction.

Exception handling and rolling back:

Also have in mind that exceptions thrown within a transaction block will be propagated (after triggering the ROLLBACK), so you should be ready to catch those in your application code.

The above statement covers the behaviour you're observing with ActiveRecord::Deadlocked with handing the exception outside of transaction block: your inner transaction propagates the exception to the outermost that Rails start for fixtures (with joinable: false), and presumably it rolls back everything, including the outermost transaction.

When you rescue the exception inside the transaction block, and the transaction handler doesn't rescue the exception, the outermost transaction has no way of knowing it has to roll back, too. I guess this is what you're aiming for exactly.

Side note: requires_new: true is an option that I strive to default to in my code.
For transactions that may have nested transactions, use joinable: false to prevent nested transactions from joining your explicit transaction. This may happen with implicit transactions ("save and destroy are automatically wrapped in a transaction") or an irresponsible code that opens transactions without requires_new: true.
Related cop suggestion.

⚠️ Beware, turning off use_transactional_tests does not selectively turn it on and off for examples if you happen to do this from before/after/around hooks. For the very brave, there's a uses_transaction option.

[sato11]

Thanks for comments. Now it's making sense to me. It didn't occur to me at all that my transaction counts as nested, but yes apparently it seems wrapped by the test example's transaction.

Turning off transactional tests (I've chosen uses_transaction for trial), the issue has apparently resolved. On the other hand the records set up by before and let! are committed as well in this case, polluting the test database. It was sort of unexpected to me at first, but this is exactly what transactional tests mean, taking the documentation into account, right?

Transactional Tests
Test cases can use begin+rollback to isolate their changes to the database instead of having to delete+insert for every test case.
https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html

[JonRowe]

Hi, yes without transactional tests you will need another database cleaning solution such as database_cleaner. As this is resolved by turning off transactional tests confirming it was the nested transactions causing the issue, I've closed this issue. The transactions are managed all by Rails test helpers.

[pirj]

I'd suggest staying away from uses_transaction unless you are testing code that is guaranteed to open the outermost transaction. This is not usually the case with application code where your code under test might be called from inside a transaction.
The burden of cleaning up manually is not much fun and as you've noticed has side effects on other tests as you've noticed.

I'd keep the code the way it was, rescuing the deadlock exception inside the inner transaction.

Specifically for your problem, retrying of deadlocks, there's deadlock_retry in the Rails org. You may want to look for some fresher forks, e.g. this one.
Not to say that there are some practices to avoid deadlocks, e.g. making updates ordering by the primary key, but this highly depends on your schema.

[sato11]

Yes I agree with everything you've mentioned. Thanks for quick advices! It was a great lesson for me that I can take advantage of hereafter 👍