Files
sure/app/models/transfer/creator.rb
T
47c46843e1 fix(transfers): prevent duplicate creation on double-submit (#3342)
* fix(transfers): prevent duplicate creation on double-submit

TransfersController#create -> Transfer::Creator had no protection
against a repeated form submission - a double-click, a browser retry,
or two near-simultaneous requests could each create a separate,
identical transfer (and its 2-4 underlying Entry/Transaction rows).

Adds a per-form idempotency key, the same approach already used for
TransactionsController#create: a UUID hidden field generated fresh on
page load, tagging the outflow/inflow (and fee, with a distinguishing
suffix since a fee leg shares its account with its primary leg) entries
via the existing entries(account_id, source, external_id) partial
unique index. A pre-check handles the sequential double-submit case;
rescue ActiveRecord::RecordNotUnique is the authoritative backstop for
genuine concurrent requests - the whole Transfer.transaction block
rolls back cleanly on conflict, so there's no risk of a half-created
transfer.

A same-day duplicate transfer can be legitimate (unlike a duplicate
valuation, see #3339/PR #3340), so this uses the same per-submission
token approach as #3334/PR #3338 rather than a natural-key DB
constraint.

Fixes #3341.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(transfers): store the idempotency key in its own column, isolate the retry with a savepoint

Same two review findings as PR #3338 (transactions) and #3340
(valuations), applied here since this branch shares the same
mechanism:

- Reusing external_id/source for the web-form idempotency token made
  every leg of a manually-created transfer satisfy Entry#linked?,
  incorrectly making it look provider-synced. Uses the same dedicated
  entries.idempotency_key column added in
  db/migrate/20260902180400_add_idempotency_key_to_entries.rb (cherry-picked
  identically from PR #3338 - this branch depends on that migration;
  please merge #3338 first, or merge this after it lands so the
  duplicate migration file is a no-op).

- Transfer::Creator now wraps the actual save in
  Transfer.transaction(requires_new: true) so a RecordNotUnique only
  rolls back to a savepoint rather than aborting any transaction the
  caller might already be in, keeping the rescue's retry lookup usable
  (mirrors the fix already applied to Account::ReconciliationManager
  in PR #3340).

Added a regression test asserting neither leg of a transfer created
via this path is linked? or has external_id/source set.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(transfers): distinct idempotency key per leg, rebuild invalid index on retry

Two more review findings:

- CodeRabbit: the form doesn't prevent selecting the same account as
  both source and destination. The outflow and inflow legs shared the
  bare idempotency key, so on that same-account path they'd collide
  with each other under the same account-scoped unique index (as
  would both fee legs, which shared a single "-fee" suffix). Every
  leg now gets a distinct, role-specific suffix (outflow stays bare -
  that's what find_existing_transfer looks up by - inflow/source_fee/
  destination_fee each get their own).

- Codex (same finding already fixed once for entries.idempotency_key's
  sibling migration, recurring here since this branch carries an
  identical copy): index_exists? alone doesn't distinguish a valid
  index from an INVALID one left behind by an interrupted CREATE INDEX
  CONCURRENTLY, so a retry after a failed build would short-circuit
  and record the migration as applied while the constraint was still
  missing. Now checks pg_index.indisvalid directly before deciding to
  skip.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(transfers): clear idempotency key on destroy so a retry doesn't 500

Codex flagged that Transfer#destroy! (used by reject!) preserves the
outflow/inflow entries but not the Transfer join row - a retried
create request with the same idempotency_key would find no Transfer
via find_existing_transfer, attempt another insert, hit the stale
entry's unique key, and re-raise RecordNotUnique instead of finding
a match. Clear the key on the surviving entries when a transfer is
destroyed.

Also adds a regression test for the CodeRabbit-flagged per-leg key
collision concern (already fixed by role-specific suffixes in the
prior commit) to lock in that fee legs never share a key with their
primary leg.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(transfers): verify idempotency key matches the request, fix stale doc comment

jjmata review on #3342:
- find_existing_transfer matched on idempotency_key + source_account only,
  so a stale key from a cached form could silently return a different,
  older transfer instead of creating the one actually requested. Now
  verifies destination account, date, and amount before treating a key
  match as the same request; a genuine mismatch surfaces as a new
  StaleIdempotencyKeyError (422 + message) instead of a false success or
  a raw 500.
- Removed a comment claiming parity with a
  TransactionsController#new_transaction_idempotency_key method that
  doesn't exist in the codebase.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(transfers): preserve from_account_id on error, match fees/exchange rate in idempotency check

coderabbitai review on #3342:
- All three create rescue blocks (exchange rate unavailable, invalid date,
  stale idempotency key) failed to set @from_account_id, so the re-rendered
  form lost the user's selected source account.
- matches_request? only compared accounts/date/outflow amount, so a retry
  with the same key but a different exchange_rate or fee would be reported
  as success while silently keeping the old inflow amount and fee entries.
  Now recomputes the request's effective inflow amount and compares derived
  fee totals too; a mismatch raises StaleIdempotencyKeyError like other
  stale-key mismatches instead of silently returning the old transfer.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
2026-09-04 06:25:34 +02:00

274 lines
11 KiB
Ruby

class Transfer::Creator
# Raised when the submitted idempotency key already belongs to an entry,
# but that entry's transfer doesn't match the current request (different
# destination/amount/date) — i.e. a stale key from a cached form rather
# than a genuine double-submit. See #find_existing_transfer.
StaleIdempotencyKeyError = Class.new(StandardError)
def initialize(family:, source_account_id:, destination_account_id:, date:, amount:, exchange_rate: nil, source_fee_amount: nil, destination_fee_amount: nil, tag_ids: nil, idempotency_key: nil)
@family = family
@source_account = family.accounts.find(source_account_id) # early throw if not found
@destination_account = family.accounts.find(destination_account_id) # early throw if not found
@date = date
@amount = amount.to_d
@source_fee_amount = source_fee_amount.to_d
@destination_fee_amount = destination_fee_amount.to_d
@tag_ids = Array(tag_ids).reject(&:blank?)
@idempotency_key = idempotency_key
if exchange_rate.present?
rate_value = exchange_rate.to_d
raise ArgumentError, "exchange_rate must be greater than 0" unless rate_value > 0
@exchange_rate = rate_value
else
@exchange_rate = nil
end
end
def create
raise ArgumentError, "source_fee_amount must be non-negative" if source_fee_amount.negative?
raise ArgumentError, "destination_fee_amount must be non-negative" if destination_fee_amount.negative?
# Sequential double-submit guard: the form was already submitted
# successfully once (double-click, browser retry, user reopening the
# dialog after a slow response) and the first request already committed
# by the time this one runs. Return the existing transfer instead of
# creating a second, identical one.
if idempotency_key && (existing_transfer = find_existing_transfer)
return existing_transfer
end
transfer = Transfer.new(
inflow_transaction: inflow_transaction,
outflow_transaction: outflow_transaction,
status: "confirmed",
amount: amount
)
# requires_new: true opens a savepoint rather than joining whatever
# transaction the caller may already be in, so a RecordNotUnique below
# only rolls back this block, not any outer transaction - keeping the
# retry lookup in the rescue usable instead of hitting
# PG::InFailedSqlTransaction (see Account::ReconciliationManager for the
# same fix applied to valuations, and the CodeRabbit/Codex findings that
# prompted it).
Transfer.transaction(requires_new: true) do
if source_fee_amount > 0
transfer.fee_transactions << build_source_fee_transaction
end
if destination_fee_amount > 0
transfer.fee_transactions << build_destination_fee_transaction
end
transfer.save!
apply_tags!(transfer) if tag_ids.any?
end
source_account.sync_later
destination_account.sync_later
transfer
rescue ActiveRecord::RecordNotUnique
# Concurrent-request backstop: two near-simultaneous submissions both
# passed the pre-check above (neither saw the other's row yet) and both
# reached #create. The partial unique index on entries(account_id,
# idempotency_key) lets exactly one leg's INSERT win, which rolls back
# this entire Transfer.transaction block (no half-created transfer left
# behind). Return the winning transfer instead of creating a duplicate
# or raising a raw DB error to the user.
#
# The INSERT hitting the unique index proves an entry with this key
# already exists on source_account, but find_existing_transfer only
# returns it when it matches the current request - so a nil here means
# the key belongs to a *different*, stale request (e.g. a cached form
# resubmitted with a new destination/amount/date), not a genuine retry.
# Surface that distinctly instead of re-raising the raw DB error.
existing_transfer = idempotency_key && find_existing_transfer
raise StaleIdempotencyKeyError unless existing_transfer
existing_transfer
end
private
attr_reader :family, :source_account, :destination_account, :date, :amount, :exchange_rate, :source_fee_amount, :destination_fee_amount, :tag_ids, :idempotency_key
# Scoped to source_account + idempotency_key so it only ever finds a
# transfer this same key could plausibly refer to, but the key alone
# isn't enough: a stale hidden field (Turbo Drive cache, reopened
# dialog) can resubmit an old key for a request that's since changed
# destination/amount/date. Verifying those fields against the request
# keeps a genuinely different transfer from being silently discarded
# in favor of returning the old one.
def find_existing_transfer
transfer = source_account.entries.find_by(idempotency_key: idempotency_key)&.entryable&.transfer
return nil unless transfer
return nil unless matches_request?(transfer)
transfer
end
# Compares against every persisted effect of #create, not just accounts
# and date/amount - a retry with the same key but a different
# exchange_rate or fee would otherwise be reported as "success" while
# silently keeping the old inflow amount and fee entries.
#
# inflow_converted_amount re-applies the current request's exchange_rate
# (or re-fetches the rate for the same date, which is cached/stable) to
# compare against what's actually persisted, rather than trying to
# recover the original request's exchange_rate from stored state (it
# isn't persisted anywhere - only its effect on the inflow amount is).
# A rate that's no longer available on retry means this isn't a
# same-request retry either, so treat that as a mismatch too and let the
# normal create path surface the real Money::ConversionError.
def matches_request?(transfer)
outflow_entry = transfer.outflow_transaction.entry
inflow_entry = transfer.inflow_transaction.entry
outflow_entry.account_id == source_account.id &&
inflow_entry.account_id == destination_account.id &&
outflow_entry.date == date &&
outflow_entry.amount == amount &&
inflow_entry.amount == inflow_converted_amount * -1 &&
transfer.derived_source_fee_amount == source_fee_amount &&
transfer.derived_destination_fee_amount == destination_fee_amount
rescue Money::ConversionError
false
end
# Every leg gets a role-specific suffix except the outflow (find_existing_transfer
# looks it up by the bare key). This matters because the unique index is
# scoped per account_id, and the form doesn't prevent selecting the same
# account as both source and destination - without distinct suffixes,
# the inflow (and a fee leg sharing its primary leg's account) would
# collide with another leg under that same account instead of with a
# genuine duplicate submission. Deliberately its own column, not
# external_id/source - see
# db/migrate/20260902180400_add_idempotency_key_to_entries.rb for why
# reusing those provider-linkage fields here would be wrong.
def entry_idempotency_attrs(leg: :outflow)
return {} unless idempotency_key
key = leg == :outflow ? idempotency_key : "#{idempotency_key}-#{leg}"
{ idempotency_key: key }
end
def apply_tags!(transfer)
resolved_ids = family.tags.where(id: tag_ids).pluck(:id)
return if resolved_ids.empty?
[ transfer.outflow_transaction, transfer.inflow_transaction ].each do |transaction|
transaction.tag_ids = resolved_ids
end
end
def outflow_transaction
name = "#{name_prefix} to #{destination_account.name}"
kind = outflow_transaction_kind
Transaction.new(
kind: kind,
category: (investment_contributions_category if kind == "investment_contribution"),
entry: source_account.entries.build(
amount: amount,
currency: source_account.currency,
date: date,
name: name,
user_modified: true,
**entry_idempotency_attrs(leg: :outflow)
)
)
end
def investment_contributions_category
source_account.family.investment_contributions_category
end
def inflow_transaction
name = "#{name_prefix} from #{source_account.name}"
net_inflow = inflow_converted_amount
Transaction.new(
kind: "funds_movement",
entry: destination_account.entries.build(
amount: net_inflow * -1,
currency: destination_account.currency,
date: date,
name: name,
user_modified: true,
**entry_idempotency_attrs(leg: :inflow)
)
)
end
def build_source_fee_transaction
fee_category = find_or_create_fees_category(source_account.family)
Transaction.new(
kind: "standard",
category: fee_category,
entry: source_account.entries.build(
amount: source_fee_amount,
currency: source_account.currency,
date: date,
name: "Transfer fee — #{name_prefix} to #{destination_account.name}",
**entry_idempotency_attrs(leg: :source_fee)
)
)
end
def build_destination_fee_transaction
fee_category = find_or_create_fees_category(destination_account.family)
Transaction.new(
kind: "standard",
category: fee_category,
entry: destination_account.entries.build(
amount: destination_fee_amount,
currency: destination_account.currency,
date: date,
name: "Transfer fee — #{name_prefix} from #{source_account.name}",
**entry_idempotency_attrs(leg: :destination_fee)
)
)
end
def find_or_create_fees_category(family)
family.categories.find_or_create_by!(name: I18n.t("models.category.defaults.fees"))
end
def inflow_converted_amount
Money.new(amount.abs, source_account.currency)
.exchange_to(
destination_account.currency,
date: date,
custom_rate: exchange_rate
).amount
end
def outflow_transaction_kind
if destination_account.loan?
"loan_payment"
elsif destination_account.liability?
"cc_payment"
elsif destination_is_investment? && !source_is_investment?
"investment_contribution"
else
"funds_movement"
end
end
def destination_is_investment?
destination_account.investment? || destination_account.crypto?
end
def source_is_investment?
source_account.investment? || source_account.crypto?
end
def name_prefix
if destination_account.liability?
"Payment"
else
"Transfer"
end
end
end