Handle after commit strategy when transactional callback of Active Record object fails before handler scheduling

[paneq]

Have a look at this Rails 4 code:

      def commit_records
        ite = records.uniq
        while record = ite.shift
          begin
            record.committed!
          rescue => e
            raise if ActiveRecord::Base.raise_in_transactional_callbacks
            record.logger.error(e) if record.respond_to?(:logger) && record.logger
          end
        end
      ensure
        ite.each do |i|
          i.committed!(false)
        end
      end

and this similar Rails 5 code:

      def commit_records
        ite = records.uniq
        while record = ite.shift
          if @run_commit_callbacks
            record.committed!
          else
            # if not running callbacks, only adds the record to the parent transaction
            record.add_to_transaction
          end
        end
      ensure
        ite.each { |i| i.committed!(should_run_callbacks: false) }
      end

and especially look at the ensure part.

Now imagine there is ActiveRecord object with after_commit callback.

Imagine that we saved this ActiveRecord object and then published an event with an async handler subscribed to it (and configured to work in after-commit fashion) and then commit a transaction. That's a perfectly normal scenario. In such case RES'es AsyncRecord is added to the list of records connected to current transaction.

Now imagine that the after_commit callback from the AR object fails and raises an exception. In such case for the rest of the records (including our AsyncRecord) in ensure part are called with committed! but passing false or should_run_callbacks: false.

Our current behavior is to ignore this false and still try to enqueue the handler. Maybe that's ok, but maybe we should follow AR convention and abort like other records do...

The benefit of current behavior is that in case of success the handler is scheduled and the situation is most likely more consistent. The downside is that if we get an exception then the exception will bubble us from ensure and the committed!(false) won't be called on the rest of the records - with unknown implications for me.

I decided to not fix it as part of #182 because the situation is less clear for me in terms of what behavior we want to have. We don't have tests for current behavior. We can add them and make this expectation explicit or we can reflect on our behavior and change it if we think that would be better.

What are your opinions?

[paneq]

Related #181

[paneq]

P.S.

Here is how we could handle it for Rails 4 & 5

def committed!(should_run_callbacks_rails_4=true, should_run_callbacks: true)
  if should_run_callbacks_rails_4 && should_run_callbacks
    @klass.perform_later(YAML.dump(@event))
  end
end

although mutation testing will probably scream 😱

[mostlyobvious]

I think we're only interested in knowing whether a transaction has been committed or rolled back:

1. If it is committed and we had a reason for scheduling (there was an event published which has a handler) then we continue with scheduling
2. If it is committed and we had no reason for scheduling then we continue no-op
3. If it is rolled back then we continue no-op

Adding a test case for the situation you described is a good idea.

[mostlyobvious]

The downside is that if we get an exception then the exception will bubble us from ensure and the committed!(false) won't be called on the rest of the records - with unknown implications for me.

1. we get an exception after transaction is committed and outside of transaction block
2. the committed!(false) will be actually called on the rest of the records