fix(transfers): prevent aborted transaction during concurrent auto-matching

[cleanjunc]

Summary

Concurrent syncs of the same family could both reach Family#auto_match_transfers! and race on the unique index of the transfers table. On PostgreSQL the losing insert aborted the whole surrounding transaction, and the existing rescue ActiveRecord::RecordNotUnique could not recover it, so the next update! raised PG::InFailedSqlTransaction and the remaining candidates were skipped.

Root cause

find_or_create_by! ran directly inside the outer Transfer.transaction. A unique violation aborts the active PostgreSQL transaction, and catching the Ruby exception does not reset that state, so every write after the rescue fails.

Fix

The insert now runs inside its own savepoint via Transfer.transaction(requires_new: true), extracted into a small find_or_create_transfer! helper. When the race is lost, only that statement rolls back to the savepoint, the RecordNotUnique is rescued, and the surrounding transaction stays healthy so the loop continues normally.

Testing

Added a regression test that reproduces a genuine unique violation during matching (a real failing insert rather than a stubbed raise, since only a real failure aborts the transaction) and asserts the run completes and still writes the transfer kinds.

bin/rails test test/models/family/auto_transfer_matchable_test.rb

Closes #2471

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability of automatic transfer matching when concurrent operations cause unique-index collisions.
  • Ensures expected “already exists” and specific “taken” validation scenarios no longer surface errors and don’t disrupt writing related entry details.

• Tests

  • Added regression coverage for race-like unique-index collisions and validation-path behavior during automatic transfer matching, including cases where unexpected validation errors should still raise.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 57659bad-ecda-43bf-a5dc-e53db875f298

📥 Commits

Reviewing files that changed from the base of the PR and between cb5e33f and f835a5f.

📒 Files selected for processing (2)

• app/models/family/auto_transfer_matchable.rb
• test/models/family/auto_transfer_matchable_test.rb

🚧 Files skipped from review as they are similar to previous changes (1)

• app/models/family/auto_transfer_matchable.rb

---

📝 Walkthrough

Walkthrough

auto_match_transfers! now delegates per-candidate transfer creation to a new private find_or_create_transfer! method. This method wraps Transfer.find_or_create_by! in a nested Transfer.transaction(requires_new: true) savepoint, so a unique-index race only rolls back the savepoint rather than aborting the outer transaction. It also handles RecordInvalid gracefully by re-raising only when the validation error is not a :taken constraint on the inflow/outflow transaction ids. Regression tests validate the fix under genuine unique-index races, validation-path races, and non-uniqueness validation failures.

Changes

Concurrent Transfer Matching Fix

Layer / File(s)	Summary
Savepoint-isolated find_or_create_transfer! helper app/models/family/auto_transfer_matchable.rb	The inline find_or_create_by! call inside the matching loop is replaced by find_or_create_transfer!(match). The new private method wraps the insert in Transfer.transaction(requires_new: true) and rescues ActiveRecord::RecordNotUnique after the savepoint, so a concurrent duplicate insert only rolls back the nested savepoint and leaves the outer transaction healthy. It also rescues RecordInvalid and re-raises unless the error is specifically a :taken validation on inflow_transaction_id or outflow_transaction_id.
Race and validation failure regression tests test/models/family/auto_transfer_matchable_test.rb	Adds three tests: (1) monkey-patches Transfer.find_or_create_by! to provoke a real unique-constraint collision and verifies auto_match_transfers! succeeds and entryable.kind values are assigned; (2) stubs find_or_create_by! to raise RecordInvalid with a :taken error on inflow_transaction_id, asserts the method does not raise and kinds are updated; (3) stubs to raise RecordInvalid with a non-:taken error and asserts the exception propagates, confirming non-uniqueness validation failures are not silently swallowed.

Sequence Diagram(s)

sequenceDiagram
  participant Job as Sync Job (loser)
  participant Matcher as auto_match_transfers!
  participant Helper as find_or_create_transfer!
  participant Outer as Transfer.transaction
  participant SP as Savepoint (requires_new: true)
  participant PG as PostgreSQL

  Job->>Matcher: auto_match_transfers!
  Matcher->>Outer: BEGIN
  loop each candidate
    Matcher->>Helper: find_or_create_transfer!(match)
    Helper->>SP: SAVEPOINT sp1
    SP->>PG: INSERT INTO transfers ...
    PG-->>SP: ERROR unique violation
    SP-->>Helper: ROLLBACK TO SAVEPOINT sp1
    Helper->>Helper: rescue RecordNotUnique
    Helper-->>Matcher: return nil
    Matcher->>PG: UPDATE entries SET kind = ...
    PG-->>Matcher: OK
  end
  Matcher->>Outer: COMMIT

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 Two syncs raced in, both wanting the prize,
A unique-index clash — oh what a surprise!
A savepoint swooped in with a rollback so neat,
The outer transaction kept rhythm and beat.
No aborts, no grief, just a bunny at peace,
With concurrent chaos granted sweet release! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: fixing a race condition that prevents transaction aborts during concurrent auto-matching of transfers.
Linked Issues check	✅ Passed	The PR fully addresses the requirements from issue #2471 by isolating the unique-index race condition in a nested transaction savepoint to prevent the surrounding transaction from being aborted.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the concurrent transfer auto-matching race condition described in the linked issue; no unrelated modifications are present.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/models/family/auto_transfer_matchable.rb`:
- Around line 78-87: The rescue block in the find_or_create_transfer! method
only catches ActiveRecord::RecordNotUnique, but the validation-path race
condition can also raise ActiveRecord::RecordInvalid when a concurrent job
commits a transfer with matching inflow_transaction_id and
outflow_transaction_id between the validation SELECT and the INSERT. Modify the
rescue clause to catch both ActiveRecord::RecordNotUnique and
ActiveRecord::RecordInvalid exceptions, handling both cases identically by doing
nothing (allowing the savepoint rollback while keeping the transaction intact),
since candidate transfers already satisfy the necessary constraints via the
matching SQL.

In `@test/models/family/auto_transfer_matchable_test.rb`:
- Around line 22-55: The test
concurrentUniqueIndexRaceDoesNotPoisonTheSurroundingTransaction currently only
exercises the RecordNotUnique error path through a real insert collision, but
does not cover the RecordInvalid path that can occur from the same race
condition referenced in the root cause code in
app/models/family/auto_transfer_matchable.rb. Add a companion test case using a
similar monkey-patch approach on Transfer.find_or_create_by! that instead
triggers a validation failure (such as creating a transfer with invalid
attributes) to ensure that the validation-path race condition also does not
poison the surrounding transaction and allows kinds to be written correctly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 87404fb4-93ff-40dd-8e4a-d1a399d81d62

📥 Commits

Reviewing files that changed from the base of the PR and between fdcd0c7 and cb5e33f.

📒 Files selected for processing (2)

• app/models/family/auto_transfer_matchable.rb
• test/models/family/auto_transfer_matchable_test.rb