From 305ef6577b108e25fb6b44d1d7d2e5a7cf4bfc3a Mon Sep 17 00:00:00 2001 From: "revise-dev[bot]" <181657630+revise-dev[bot]@users.noreply.github.com> Date: Fri, 31 Jan 2025 11:12:32 +0000 Subject: [PATCH] Fix: PG::TRDeadlockDetected (MAYBE-RAILS-DC) --- app/models/transfer.rb | 70 +++++++++++++++++++++------- spec/models/transfer_spec.rb | 90 ++++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 17 deletions(-) create mode 100644 spec/models/transfer_spec.rb diff --git a/app/models/transfer.rb b/app/models/transfer.rb index 3cb1f07bcfc..50e806e7db5 100644 --- a/app/models/transfer.rb +++ b/app/models/transfer.rb @@ -14,16 +14,16 @@ class Transfer < ApplicationRecord class << self def from_accounts(from_account:, to_account:, date:, amount:) - # Attempt to convert the amount to the to_account's currency. - # If the conversion fails, use the original amount. - converted_amount = begin - Money.new(amount.abs, from_account.currency).exchange_to(to_account.currency) - rescue Money::ConversionError - Money.new(amount.abs, from_account.currency) - end - - new( - inflow_transaction: Account::Transaction.new( + Transaction.transaction do + # Attempt to convert the amount to the to_account's currency. + # If the conversion fails, use the original amount. + converted_amount = begin + Money.new(amount.abs, from_account.currency).exchange_to(to_account.currency) + rescue Money::ConversionError + Money.new(amount.abs, from_account.currency) + end + + inflow_transaction = Account::Transaction.new( entry: to_account.entries.build( amount: converted_amount.amount.abs * -1, currency: converted_amount.currency.iso_code, @@ -31,8 +31,9 @@ def from_accounts(from_account:, to_account:, date:, amount:) name: "Transfer from #{from_account.name}", entryable: Account::Transaction.new ) - ), - outflow_transaction: Account::Transaction.new( + ) + + outflow_transaction = Account::Transaction.new( entry: from_account.entries.build( amount: amount.abs, currency: from_account.currency, @@ -40,21 +41,50 @@ def from_accounts(from_account:, to_account:, date:, amount:) name: "Transfer to #{to_account.name}", entryable: Account::Transaction.new ) - ), - status: "confirmed" - ) + ) + + transfer = new( + inflow_transaction: inflow_transaction, + outflow_transaction: outflow_transaction, + status: "confirmed" + ) + + # Save all records within the transaction + inflow_transaction.save! + outflow_transaction.save! + transfer.save! + transfer + end + end + + def create_transfer!(inflow_transaction_id:, outflow_transaction_id:) + transaction do + _lock_transactions!(inflow_transaction_id, outflow_transaction_id) + create!( + inflow_transaction_id: inflow_transaction_id, + outflow_transaction_id: outflow_transaction_id, + status: 'pending' + ) + end end end def reject! Transfer.transaction do - RejectedTransfer.find_or_create_by!(inflow_transaction_id: inflow_transaction_id, outflow_transaction_id: outflow_transaction_id) + _lock_transactions!(inflow_transaction_id, outflow_transaction_id) + RejectedTransfer.find_or_create_by!( + inflow_transaction_id: inflow_transaction_id, + outflow_transaction_id: outflow_transaction_id + ) destroy! end end def confirm! - update!(status: "confirmed") + Transfer.transaction do + _lock_transactions!(inflow_transaction_id, outflow_transaction_id) + update!(status: "confirmed") + end end def sync_account_later @@ -95,6 +125,12 @@ def categorizable? end private + def _lock_transactions!(transaction_id1, transaction_id2) + [transaction_id1, transaction_id2].sort.each do |transaction_id| + Account::Transaction.lock.find(transaction_id) + end + end + def transfer_has_different_accounts return unless inflow_transaction.present? && outflow_transaction.present? errors.add(:base, :must_be_from_different_accounts) if inflow_transaction.entry.account == outflow_transaction.entry.account diff --git a/spec/models/transfer_spec.rb b/spec/models/transfer_spec.rb new file mode 100644 index 00000000000..201532eaeb8 --- /dev/null +++ b/spec/models/transfer_spec.rb @@ -0,0 +1,90 @@ +require 'rails_helper' + +RSpec.describe Transfer, type: :model do + let(:family) { create(:family) } + let(:from_account) { create(:account, family: family) } + let(:to_account) { create(:account, family: family) } + + describe 'concurrent operations' do + let!(:inflow_transaction) { create(:account_transaction) } + let!(:outflow_transaction) { create(:account_transaction) } + + it 'handles concurrent transfer creation without deadlocks' do + threads = [] + expect do + 2.times do + threads << Thread.new do + Transfer.create_transfer!( + inflow_transaction_id: inflow_transaction.id, + outflow_transaction_id: outflow_transaction.id + ) + end + end + threads.each(&:join) + end.not_to raise_error + + expect(Transfer.count).to eq(1) + end + + context 'when rejecting transfers concurrently' do + let!(:transfer) { create(:transfer, inflow_transaction: inflow_transaction, outflow_transaction: outflow_transaction) } + + it 'handles concurrent rejections without deadlocks' do + threads = [] + expect do + 2.times do + threads << Thread.new do + begin + transfer.reject! + rescue ActiveRecord::RecordNotFound + # Expected when another thread has already rejected + end + end + end + threads.each(&:join) + end.not_to raise_error + + expect(Transfer.exists?(transfer.id)).to be false + expect(RejectedTransfer.count).to eq(1) + end + end + + it 'handles mixed operations without deadlocks' do + transfer = create(:transfer, inflow_transaction: inflow_transaction, outflow_transaction: outflow_transaction) + + threads = [] + expect do + threads << Thread.new do + transfer.reject! + end + + threads << Thread.new do + Transfer.create_transfer!( + inflow_transaction_id: inflow_transaction.id, + outflow_transaction_id: outflow_transaction.id + ) + end + + threads.each(&:join) + end.not_to raise_error + end + end + + describe '.from_accounts' do + it 'creates a transfer with consistent locking' do + transfer = nil + expect do + transfer = Transfer.from_accounts( + from_account: from_account, + to_account: to_account, + date: Date.current, + amount: Money.new(1000, 'USD') + ) + end.not_to raise_error + + expect(transfer).to be_persisted + expect(transfer.inflow_transaction).to be_persisted + expect(transfer.outflow_transaction).to be_persisted + end + end +end