Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corrupt transaction counts with Rails 7.1 restarting savepoint transactions #65

Closed
composerinteralia opened this issue Aug 31, 2023 · 1 comment · Fixed by #69
Closed
Assignees

Comments

@composerinteralia
Copy link

composerinteralia commented Aug 31, 2023

This is similar to #64, but I'm filing it separately since this one really is a bug.

What did you do?

I tried to run isolator at GitHub and ran into some Trying to finalize an untracked transaction warnings. Here's a failing test (against Rails main) to demonstrate the issue:

    context "when rolling back a restarting savepoint transaction" do
      specify do
        expect(Isolator).to_not be_within_transaction

        begin
          ActiveRecord::Base.logger = Logger.new(STDOUT)
          # RealTransaction (begin..rollback)
          ar_class.transaction do
            ar_class.first
            # Savepoint Transaction (savepoint..rollback)
            ar_class.transaction(requires_new: true) do
              # ResetParentTransaction (rollback to outer savepoint)
              ar_class.transaction(requires_new: true) do
                ar_class.first
                expect(Isolator).to be_within_transaction
                raise "Rollback"
              end
            end
          ensure
            expect(Isolator).to be_within_transaction # Oops, transaction count is already 0 so this test fails
          end
        rescue
        end

        expect(Isolator).to_not be_within_transaction
      end
    end

This happens because of rails/rails#44526, which can cause the same savepoint to get rolled back more than once. In older versions of Rails the transaction queries were:

BEGIN
SAVEPOINT active_record_1
SAVEPOINT active_record_2
ROLLBACK TO SAVEPOINT active_record_2
ROLLBACK TO SAVEPOINT active_record_1
ROLLBACK

But in Rails 7.1 it becomes:

BEGIN
SAVEPOINT active_record_1
ROLLBACK TO SAVEPOINT active_record_1
ROLLBACK TO SAVEPOINT active_record_1
ROLLBACK

That's 2 starts and 3 finishes, so we end up decrementing the count too far.

Environment

Ruby Version:

3.2

Framework Version (Rails, whatever):

Rails 7.1 (unreleased)

Isolator Version:

0.10.0

composerinteralia added a commit to composerinteralia/rails that referenced this issue Sep 7, 2023
Tracking Active Record-managed transactions seems to be a common need,
but there's currently not a great way to do it. Here's a few examples
I've seen:

* GitHub has custom transaction tracking that monkey patches the Active
  Record `TransactionManager` and `RealTransaction`. We use the tracking
  to prevent opening a transaction to one database cluster inside a
  transaction to a different database cluster, and to report slow
  transactions (we get slow transaction data directly from MySQL as well,
  but it's still helpful to report from the application with backtraces to
  help track them down).
* https://github.com/palkan/isolator tracks transactions to prevent
  non-atomic interactions like external network calls inside a
  transaction. The gem works by subscribing to `sql.active_record`, then
  piecing together the transactions by looking for `BEGIN`, `COMMIT`,
  `SAVEPOINT`, etc., but this is unreliable:
    - palkan/isolator#65
    - palkan/isolator#64
* It looks like GitLab patches `TransactionManager` and `RealTransaction`
  to track nested savepoints. See palkan/isolator#46

This commit adds a new `transaction.active_record` event that should
provide a more reliable solution for these various use cases. It
includes the connection in the payload (useful, for example, in
differentiating transactions to different databases), but if this change
gets merged we're also planning to add details about what type of
transaction it is (savepoint or real) and what the outcome is (commit,
rollback, restarted, errored).

This instrumentation needs to start and finish at fairly specific times:
  - start on materialize
  - finish after committing or rolling back, but before the after_commit
    or after_rollback callbacks
  - finish and start again when the transaction restarts (at least for
    real transactions—we've done it for savepoints as well but I'm not
    certain we should)
  - ensure it finishes if commit and rollback fail (e.g. if the
    connection goes away)
To make all that work, this commit uses the lower-level `#build-handle`
API instead of `#instrument`.

Co-authored-by: Ian Candy <ipc103@github.com>
ipc103 added a commit to ipc103/rails that referenced this issue Sep 12, 2023
Tracking Active Record-managed transactions seems to be a common need,
but there's currently not a great way to do it. Here's a few examples
I've seen:

* GitHub has custom transaction tracking that monkey patches the Active
  Record `TransactionManager` and `RealTransaction`. We use the tracking
  to prevent opening a transaction to one database cluster inside a
  transaction to a different database cluster, and to report slow
  transactions (we get slow transaction data directly from MySQL as well,
  but it's still helpful to report from the application with backtraces to
  help track them down).
* https://github.com/palkan/isolator tracks transactions to prevent
  non-atomic interactions like external network calls inside a
  transaction. The gem works by subscribing to `sql.active_record`, then
  piecing together the transactions by looking for `BEGIN`, `COMMIT`,
  `SAVEPOINT`, etc., but this is unreliable:
    - palkan/isolator#65
    - palkan/isolator#64
* It looks like GitLab patches `TransactionManager` and `RealTransaction`
  to track nested savepoints. See palkan/isolator#46

This commit adds a new `transaction.active_record` event that should
provide a more reliable solution for these various use cases. It
includes the connection in the payload (useful, for example, in
differentiating transactions to different databases), but if this change
gets merged we're also planning to add details about what type of
transaction it is (savepoint or real) and what the outcome is (commit,
rollback, restarted, errored).

This instrumentation needs to start and finish at fairly specific times:
  - start on materialize
  - finish after committing or rolling back, but before the after_commit
    or after_rollback callbacks
  - finish and start again when the transaction restarts (at least for
    real transactions—we've done it for savepoints as well but I'm not
    certain we should)
  - ensure it finishes if commit and rollback fail (e.g. if the
    connection goes away)
To make all that work, this commit uses the lower-level `#build-handle`
API instead of `#instrument`.

Co-authored-by: Ian Candy <ipc103@github.com>
@composerinteralia
Copy link
Author

rails/rails#49192 was merged, so that might be a more reliable option for the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants