From cb5e33f1a673d0784c09776f5ad18416bb8702ff Mon Sep 17 00:00:00 2001 From: cleanjunc Date: Tue, 23 Jun 2026 09:54:08 +0000 Subject: [PATCH 1/2] fix(transfers): isolate auto-match insert in a savepoint to survive concurrent races --- app/models/family/auto_transfer_matchable.rb | 22 +++++++----- .../family/auto_transfer_matchable_test.rb | 35 +++++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/app/models/family/auto_transfer_matchable.rb b/app/models/family/auto_transfer_matchable.rb index 9ff6395fdc..bddee50d0f 100644 --- a/app/models/family/auto_transfer_matchable.rb +++ b/app/models/family/auto_transfer_matchable.rb @@ -43,14 +43,7 @@ def auto_match_transfers!(account: nil) next if used_transaction_ids.include?(match.inflow_transaction_id) || used_transaction_ids.include?(match.outflow_transaction_id) - begin - Transfer.find_or_create_by!( - inflow_transaction_id: match.inflow_transaction_id, - outflow_transaction_id: match.outflow_transaction_id, - ) - rescue ActiveRecord::RecordNotUnique - # Another concurrent job created the transfer; safe to ignore - end + find_or_create_transfer!(match) inflow_transaction = transactions_by_id.fetch(match.inflow_transaction_id) outflow_transaction = transactions_by_id.fetch(match.outflow_transaction_id) @@ -80,6 +73,19 @@ def auto_match_transfers!(account: nil) end private + # Isolate the insert in a savepoint so losing the unique-index race to a + # concurrent job rolls back only this statement, not the surrounding transaction. + def find_or_create_transfer!(match) + Transfer.transaction(requires_new: true) do + Transfer.find_or_create_by!( + inflow_transaction_id: match.inflow_transaction_id, + outflow_transaction_id: match.outflow_transaction_id, + ) + end + rescue ActiveRecord::RecordNotUnique + # Concurrent job already created it; savepoint rolled back, transaction intact. + end + def coerce_transfer_match_date_window!(value) Integer(value) rescue ArgumentError, TypeError diff --git a/test/models/family/auto_transfer_matchable_test.rb b/test/models/family/auto_transfer_matchable_test.rb index 5951d94876..a846ac255f 100644 --- a/test/models/family/auto_transfer_matchable_test.rb +++ b/test/models/family/auto_transfer_matchable_test.rb @@ -19,6 +19,41 @@ class Family::AutoTransferMatchableTest < ActiveSupport::TestCase end end + test "concurrent unique-index race does not poison the surrounding transaction" do + outflow_entry = create_transaction(date: 1.day.ago.to_date, account: @depository, amount: 500) + inflow_entry = create_transaction(date: Date.current, account: @credit_card, amount: -500) + inflow_id = inflow_entry.entryable_id + + # An existing transfer we can collide with to trigger a real unique violation, + # mimicking a concurrent job that won the race. A genuine failing INSERT (not a + # synthetic raise) is what aborts the transaction, so only this reproduces the bug. + rival_out = create_transaction(date: 1.day.ago.to_date, account: @depository, amount: 250) + rival_in = create_transaction(date: Date.current, account: @credit_card, amount: -250) + rival = Transfer.create!(inflow_transaction_id: rival_in.entryable_id, outflow_transaction_id: rival_out.entryable_id) + + original = Transfer.method(:find_or_create_by!) + Transfer.singleton_class.send(:define_method, :find_or_create_by!) do |attributes| + if attributes[:inflow_transaction_id] == inflow_id + insert!(inflow_transaction_id: rival.inflow_transaction_id, outflow_transaction_id: rival.outflow_transaction_id) + else + original.call(attributes) + end + end + + begin + assert_nothing_raised { @family.auto_match_transfers! } + ensure + Transfer.singleton_class.send(:remove_method, :find_or_create_by!) + end + + inflow_entry.reload + outflow_entry.reload + + # Surrounding transaction stayed healthy, so kinds were still written. + assert_equal "funds_movement", inflow_entry.entryable.kind + assert_equal "cc_payment", outflow_entry.entryable.kind + end + test "auto-matches multi-currency transfers" do load_exchange_prices create_transaction(date: 1.day.ago.to_date, account: @depository, amount: 500) From f835a5f95f66ff52ed9e663c3e92386aa9e0b95a Mon Sep 17 00:00:00 2001 From: cleanjunc Date: Tue, 23 Jun 2026 11:11:26 +0000 Subject: [PATCH 2/2] fix(transfers): handle validation-path race in auto-match transfer creation --- app/models/family/auto_transfer_matchable.rb | 5 ++- .../family/auto_transfer_matchable_test.rb | 32 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/app/models/family/auto_transfer_matchable.rb b/app/models/family/auto_transfer_matchable.rb index bddee50d0f..26aabfa94d 100644 --- a/app/models/family/auto_transfer_matchable.rb +++ b/app/models/family/auto_transfer_matchable.rb @@ -83,7 +83,10 @@ def find_or_create_transfer!(match) ) end rescue ActiveRecord::RecordNotUnique - # Concurrent job already created it; savepoint rolled back, transaction intact. + # Lost the insert race; savepoint rolled back, transaction intact. + rescue ActiveRecord::RecordInvalid => e + # Same race caught by the uniqueness validation; re-raise anything else. + raise unless %i[inflow_transaction_id outflow_transaction_id].any? { |attr| e.record.errors.of_kind?(attr, :taken) } end def coerce_transfer_match_date_window!(value) diff --git a/test/models/family/auto_transfer_matchable_test.rb b/test/models/family/auto_transfer_matchable_test.rb index a846ac255f..3d6ce79386 100644 --- a/test/models/family/auto_transfer_matchable_test.rb +++ b/test/models/family/auto_transfer_matchable_test.rb @@ -54,6 +54,38 @@ class Family::AutoTransferMatchableTest < ActiveSupport::TestCase assert_equal "cc_payment", outflow_entry.entryable.kind end + test "validation-path race during matching does not abort the run" do + outflow_entry = create_transaction(date: 1.day.ago.to_date, account: @depository, amount: 500) + inflow_entry = create_transaction(date: Date.current, account: @credit_card, amount: -500) + + # A concurrent job already matched one of these transactions, so the uniqueness + # validation rejects the insert with :taken (a plain raise, no aborted SQL). + invalid = Transfer.new(inflow_transaction_id: inflow_entry.entryable_id, outflow_transaction_id: outflow_entry.entryable_id) + invalid.errors.add(:inflow_transaction_id, :taken) + Transfer.stubs(:find_or_create_by!).raises(ActiveRecord::RecordInvalid.new(invalid)) + + assert_nothing_raised { @family.auto_match_transfers! } + + inflow_entry.reload + outflow_entry.reload + + # The duplicate was swallowed, so the loop kept running and wrote kinds. + assert_equal "funds_movement", inflow_entry.entryable.kind + assert_equal "cc_payment", outflow_entry.entryable.kind + end + + test "non-uniqueness validation failure during matching is not swallowed" do + create_transaction(date: 1.day.ago.to_date, account: @depository, amount: 500) + create_transaction(date: Date.current, account: @credit_card, amount: -500) + + # A genuine (non-race) validation failure must surface, not be skipped silently. + invalid = Transfer.new + invalid.errors.add(:base, :different_accounts) + Transfer.stubs(:find_or_create_by!).raises(ActiveRecord::RecordInvalid.new(invalid)) + + assert_raises(ActiveRecord::RecordInvalid) { @family.auto_match_transfers! } + end + test "auto-matches multi-currency transfers" do load_exchange_prices create_transaction(date: 1.day.ago.to_date, account: @depository, amount: 500)