Repository navigation
save returns true when a before_commit callback rolls the transaction back #59439
Description
with_transaction_returning_status stores the result of the persistence block in status and returns it once the implicit transaction is done. A before_commit callback runs after that block, while the transaction commits. If it raises ActiveRecord::Rollback, the transaction is rolled back and transaction swallows the exception, but status is already true, so save returns true for a record that was never written.
The record state is restored correctly (persisted? is false), so only the return value is wrong. destroy returns the record in the same situation and destroy! does not raise. The method's own comment says "If the status is true, the transaction is committed", and a Rollback raised from after_save already makes save return nil, as accepted in #5527. This is about the commit phase only.
Steps to reproduce
# frozen_string_literal: true
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
gem "rails"
# If you want to test against edge Rails replace the previous line with this:
# gem "rails", github: "rails/rails", branch: "main"
gem "sqlite3"
end
require "active_record/railtie"
require "minitest/autorun"
# This connection will do for database-independent bug reports.
ENV["DATABASE_URL"] = "sqlite3::memory:"
class TestApp < Rails::Application
config.load_defaults Rails::VERSION::STRING.to_f
config.eager_load = false
config.logger = Logger.new($stdout)
config.secret_key_base = "secret_key_base"
config.active_record.encryption.primary_key = "primary_key"
config.active_record.encryption.deterministic_key = "deterministic_key"
config.active_record.encryption.key_derivation_salt = "key_derivation_salt"
end
Rails.application.initialize!
ActiveRecord::Schema.define do
create_table :posts, force: true do |t|
t.string :title
end
end
class Post < ActiveRecord::Base
before_commit { raise ActiveRecord::Rollback }
end
class BugTest < ActiveSupport::TestCase
setup { Post.delete_all }
def test_save_reports_failure_when_before_commit_rolls_back
post = Post.new(title: "Hello")
assert_not post.save
assert_not post.persisted?
assert_equal 0, Post.count
end
def test_destroy_bang_raises_when_before_commit_rolls_back
Post.insert!({ title: "Hello" })
post = Post.first
assert_raises(ActiveRecord::RecordNotDestroyed) { post.destroy! }
assert_equal 1, Post.count
end
endExpected behavior
save returns a falsy value and destroy! raises ActiveRecord::RecordNotDestroyed, the same as when the Rollback comes from after_save or after_destroy.
Actual behavior
1) Failure:
BugTest#test_destroy_bang_raises_when_before_commit_rolls_back:
ActiveRecord::RecordNotDestroyed expected but nothing was raised.
2) Failure:
BugTest#test_save_reports_failure_when_before_commit_rolls_back:
Expected true to be nil or false
2 runs, 2 assertions, 2 failures, 0 errors, 0 skips
The same failures reproduce on main (5b02449). Inside an explicit transaction block, save still returns true and the outer transaction call returns nil, which is correct because that commit hasn't happened yet.
I have a small local fix with tests and can open a PR.
System configuration
Rails version: 8.1.4, and main at 5b02449
Ruby version: 4.0.9
Activity
Run before_commit rollback tests without transactional tests
Remove comment above the before_commit status check
Return falsy from save when before_commit rolls back
Run before_commit rollback tests without transactional tests
Remove comment above the before_commit status check