Make after_commit work reliabliy inside nested transactions

[matid]

In the current implementation, after_commit callbacks are ran
immediately after the ActiveRecord::Base.transaction block,
which is not guaranteed to be after the transaction has been
committed to the database.

Speficially, if statesman code is running inside another transaction,
the after_commit block will be running prematurely.

This commit fixes that behavious by using ActiveRecord’s
add_transaction_record method (which is how after_commit
callbacks are implemented in Rails).

This issue was already reported in #281 in December 2017, but never fixed.

Here’s a blog post explaining this behaviour: https://dev.to/evilmartians/rails-aftercommit-everywhere--4j9g

A secondary side-effect of this behaviour is that if you schedule a background job from within the after_commit: true callback, it’s not guaranteed that the transition has been committed to the database yet, and the scheduled job might not run correctly.

This is a sample state machine exhibiting this behaviour:

class OrderStateMachine
  include Statesman::Machine

  state :pending, initial: true
  state :placed

  transition from: :pending, to: :placed

  after_transition from: :pending, to: :placed do
    puts "after_transition(after_commit: false) callback"
  end

  after_transition from: :pending, to: :placed, after_commit: true do
    puts "after_transition(after_commit: true) callback"
  end
end

and a simple test case:

ActiveRecord::Base.transaction do
  order = Order.first
  order.state_machine.transition_to! :placed
  raise
end

Without the patch, the after_commit callback will execute even if the transaction is rolled back:

irb(main):001:0> Runner.run
   (0.1ms)  BEGIN
  Order Load (0.5ms)  SELECT  "orders".* FROM "orders" ORDER BY "orders"."id" ASC LIMIT $1  [["LIMIT", 1]]
  OrderTransition Load (0.7ms)  SELECT  "order_transitions".* FROM "order_transitions" WHERE "order_transitions"."order_id" = $1 ORDER BY "order_transitions"."sort_key" DESC LIMIT $2  [["order_id", 1], ["LIMIT", 1]]
  OrderTransition Load (0.3ms)  SELECT  "order_transitions".* FROM "order_transitions" WHERE "order_transitions"."order_id" = $1 ORDER BY "order_transitions"."sort_key" DESC LIMIT $2  [["order_id", 1], ["LIMIT", 1]]
  OrderTransition Update All (2.9ms)  UPDATE "order_transitions" SET "most_recent" = FALSE, "updated_at" = '2019-02-03 18:57:18.133758' WHERE "order_transitions"."order_id" = $1 AND "order_transitions"."most_recent" = $2  [["order_id", 1], ["most_recent", true]]
  OrderTransition Create (23.3ms)  INSERT INTO "order_transitions" ("to_state", "sort_key", "order_id", "most_recent", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5, $6) RETURNING "id"  [["to_state", "placed"], ["sort_key", 10], ["order_id", 1], ["most_recent", true], ["created_at", "2019-02-03 18:57:18.138074"], ["updated_at", "2019-02-03 18:57:18.138074"]]
after_transition(after_commit: false) callback
after_transition(after_commit: true) callback
   (0.4ms)  ROLLBACK
Traceback (most recent call last):
        3: from (irb):1
        2: from app/models/runner.rb:3:in `run'
        1: from app/models/runner.rb:6:in `block in run'
RuntimeError ()

With the patch, only the after_commit: false callback will run:

   (0.2ms)  BEGIN
  Order Load (0.4ms)  SELECT  "orders".* FROM "orders" ORDER BY "orders"."id" ASC LIMIT $1  [["LIMIT", 1]]
  OrderTransition Load (0.6ms)  SELECT  "order_transitions".* FROM "order_transitions" WHERE "order_transitions"."order_id" = $1 ORDER BY "order_transitions"."sort_key" DESC LIMIT $2  [["order_id", 1], ["LIMIT", 1]]
  OrderTransition Load (0.3ms)  SELECT  "order_transitions".* FROM "order_transitions" WHERE "order_transitions"."order_id" = $1 ORDER BY "order_transitions"."sort_key" DESC LIMIT $2  [["order_id", 1], ["LIMIT", 1]]
  OrderTransition Update All (0.7ms)  UPDATE "order_transitions" SET "most_recent" = FALSE, "updated_at" = '2019-02-03 18:58:06.566445' WHERE "order_transitions"."order_id" = $1 AND "order_transitions"."most_recent" = $2  [["order_id", 1], ["most_recent", true]]
  OrderTransition Create (1.1ms)  INSERT INTO "order_transitions" ("to_state", "sort_key", "order_id", "most_recent", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5, $6) RETURNING "id"  [["to_state", "placed"], ["sort_key", 10], ["order_id", 1], ["most_recent", true], ["created_at", "2019-02-03 18:58:06.568712"], ["updated_at", "2019-02-03 18:58:06.568712"]]
after_transition(after_commit: false) callback
   (0.3ms)  ROLLBACK
Traceback (most recent call last):
        3: from (irb):1
        2: from app/models/runner.rb:3:in `run'
        1: from app/models/runner.rb:6:in `block in run'
RuntimeError ()

[danwakefield]

@matid This looks really good.
I've fixed the issue with our test runner so if you rebase we'll be able to see if it works and think about getting a new major release out.

[matid]

Thanks @danwakefield. I fixed the Rubocop warnings and it looks like everything is green now.

[danwakefield]

@matid Sorry I haven't gotten back to you.
I used your example as a template for a spec but I can't seem to get it to fail
See #341

I was going to release your change along with
#249 but it seems like using real transactions might have solved the issue here too.

The testcase is a bit funky so I could be wrong. What do you think? If that solves things I'm happy to cut a release for it.

[matid]

@danwakefield Your test case is succeeding because your side-effect (MyActiveRecordModel.create) that you’re checking for gets rolled back when the top-level transaction fails. The code still gets executed.

This is a test case that will fail without my patch (even with #249):

context "placeholder" do
  before do
    MyStateMachine.class_eval do
      cattr_accessor(:after_commit_callback_executed) { false }

      after_transition(from: :initial, to: :succeeded, after_commit: true) do
        MyStateMachine.after_commit_callback_executed = true
      end
    end
  end

  after do
    MyStateMachine.class_eval do
      callbacks[:after_commit] = []
    end
  end

  let!(:model) do
    MyActiveRecordModel.create
  end

  # rubocop:disable RSpec/ExampleLength
  it do
    expect do
      ActiveRecord::Base.transaction do
        model.state_machine.transition_to!(:succeeded)
        raise ActiveRecord::Rollback
      end
    end.to_not change(MyStateMachine, :after_commit_callback_executed)
  end
  # rubocop:enable RSpec/ExampleLength
end

[matid]

@danwakefield I committed an updated version of the after commit wrapper that works correctly with the transaction(requires_new: true).

The test case above should fail on master without the patch, and pass once my changes are merged in.

[danwakefield]

Perfect, I suspected I wasn't testing it properly. I'll merge and release today.
Thanks for contributing.

[matid]

Thanks @danwakefield, appreciate it!

FYI: This pull request also closes #281.