Run transactional callbacks on instances most likely to match DB state

[cbothner]

When multiple instances of the same ActiveRecord model are used to persist changes within a transaction, Rails always runs after_commit callbacks on the first to save. This results in very unintuitive behavior:

Product.transaction do
  Product.find(id).update!(title: "T-Shirt")
  Product.find(id).update!(description: "A cool T-shirt")
end
# When the transaction commits, `after_update_commit` callbacks are fired
# on the first instance, which does not have the new description.

Product.transaction do
  Product.find(id).update!(title: "T-Shirt")
  Product.find(id).destroy!
end
# When the transaction commits, `after_update_commit` callbacks are fired,
# even though the product is destroyed.

This PR implements a change to instead fire transactional callbacks on instances with internal state most likely to match the committed database state.

• In general, run transactional callbacks on the last instance to save a given record within the transaction.
• There are two exceptions:
  • If the record is created within the transaction, then updated by another instance, after_create_commit callbacks will be run on the second instance. This is instead of the after_update_commit callbacks that would naively be run based on that instance’s state.
  • If the record is destroyed within the transaction, then after_destroy_commit callbacks will be fired on the last destroyed instance, even if a stale instance subsequently performed an update (which will have affected 0 rows).

Please see the added test cases for a more detailed statement of behavior.

[rafaelfranca]

This looks good to me. Let's add the flag to allow people to opt-in to it, but still makes this a framework default

[cbothner]

@rafaelfranca I added the configuration flag, updated the framework defaults, adding documentation for the new flag, and added a changelog entry.

I considered whether a deprecation warning was necessary but based on the example of other such framework defaults I decided not to. We aren't really deprecating the old behavior if there's a configuration flag that lets an app developer opt in or out as they please. Let me know what you think.

CI is red but as far as I can tell it appears just to be flakiness.

[rafaelfranca]

Is this an exception? It is running the callback on the last instance to save.

[cbothner]

The exceptional behavior is that we're running after_create_commit, not after_update_commit on that instance, even though from its perspective it just issued an update

https://github.com/rails/rails/blob/ad54b7a6733373b5426ceed6c4663f99e9781dbb/activerecord/test/cases/transaction_callbacks_test.rb#L791-L802

If you don't feel like that detail needs to be called out, then I'm happy to remove or reword.

[rafaelfranca]

Ahhh, got it, that is not clear in the message. Can we update the message to show that difference?

[cbothner]

Okay, I updated it. Is that more clear?

[kshnurov]

@cbothner @rafaelfranca this change silently breaks ActiveModel::Dirty for after_commit callbacks. saved_changes, saved_change_to_x? and etc are pretty common in after_commit.

Depending on what else is happening in the transaction they'll either work as expected or will be completely ignored since the last object won't have the expected changes. A nightmare to debug and the only way to fix is to disable this behavior.

[cbothner]

@kshnurov Anything that relies on instance state is unreliable when two instances are being used to write to the same row in the same transaction. No matter which instance we choose for callbacks, we'll miss something. Between the two options, I believe this is the better choice. Missing dirty state from the first instance to save does break callbacks that are conditional on saved_changes, but we provide the right column data to callbacks that don't rely on dirty state. Missing database state from the second to save breaks all callbacks that rely on that state, and sometimes results in the wrong lifecycle event being fired.

Really I think we should all try not to get into situations where we have multiple instances representing the same row. Passing around one instance will ensure no state is lost, and as a bonus it will save unnecessary SELECT statements.

[kshnurov]

@cbothner well, both are bad, but the stale state is an expected behaviour described in the docs. This PR didn't update them, which makes things even worse - people will expect it to work as before.

This one is a breaking change in Rails 7.1 that leads to silent hard-to-find bugs.
Fixing something while breaking the other thing is not a good solution.
Stale state is known, expected and could be easily fixed by a simple .reload. I don't see a workaround for this one though.

[zzak]

@kshnurov Sorry to dismiss your comments here, but people make mistakes and responding to a PR that was merged over 18 months ago probably won't get the feedback you're looking for. I suggest if you can create a reproduction to open a new issue. 🙏