fix(recurring): match transfer pairs so Cleaner stops mis-retiring transfers (#2110)

Closes #1590. Implements Option A (the proper fix), replacing the interim skip.

A recurring transfer's name is seeded as "Transfer to {dest}", but future
occurrences carry arbitrary names (user free-text, importer wording, the
auto-matcher), so the name-based matching_transactions returned [] and the
Cleaner retired still-active transfers at the 6-month threshold. main worked
around this by skipping transfer rows entirely (Option B) — which also meant a
genuinely-stopped transfer never got retired.

matching_transactions now detects the Transfer *pair* for transfer rows: an
outflow on the source account paired with an inflow on the destination account,
within the usual amount/cadence window. The Cleaner no longer skips transfers:

- a transfer whose pair still occurs keeps surfacing recent matches → stays active
- a transfer whose pair has stopped → correctly retired

The amount / day-of-month scopes are extracted and shared between the name-based
and pair-based paths. The Identifier's separate transfer skip (auto-identifying
pairs from history) is intentionally untouched — that's the out-of-scope feature
the issue defers.

app/models/recurring_transaction.rb

@@ -260,29 +260,15 @@ class RecurringTransaction < ApplicationRecord
 
   # Find matching transactions for this recurring pattern
   def matching_transactions
-    # For manual recurring with amount variance, match within range
-    # For automatic recurring, match exact amount
-    base = account.present? ? account.entries : family.entries
+    # Recurring transfers can't be matched by single-account name/amount —
+    # future occurrences carry arbitrary names — so match the Transfer pair.
+    return transfer_matching_transactions if transfer?
 
-    entries = if manual? && has_amount_variance?
-      base
-        .where(entryable_type: "Transaction")
-        .where(currency: currency)
-        .where("entries.amount BETWEEN ? AND ?", expected_amount_min, expected_amount_max)
-        .where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?",
-               [ expected_day_of_month - 2, 1 ].max,
-               [ expected_day_of_month + 2, 31 ].min)
-        .order(date: :desc)
-    else
-      base
-        .where(entryable_type: "Transaction")
-        .where(currency: currency)
-        .where("entries.amount = ?", amount)
-        .where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?",
-               [ expected_day_of_month - 2, 1 ].max,
-               [ expected_day_of_month + 2, 31 ].min)
-        .order(date: :desc)
-    end
+    # Amount/cadence-scoped Transaction entries on this account (or family).
+    base = account.present? ? account.entries : family.entries
+    entries = day_of_month_scope(
+      amount_window_scope(base.where(entryable_type: "Transaction").where(currency: currency))
+    ).order(date: :desc)
 
     # Filter by merchant or name
     if merchant_id.present?
@@ -401,6 +387,47 @@ class RecurringTransaction < ApplicationRecord
   end
 
   private
+    # Issue #1590: a recurring transfer's future occurrences rarely share the
+    # seed's name (user free-text, importer wording, the auto-matcher's
+    # "Transfer to ..."), so name-based matching returns [] and the Cleaner
+    # would wrongly inactivate a still-active transfer. Match the Transfer
+    # *pair* instead — an outflow on the source account paired with an inflow
+    # on the destination account, within the usual amount/cadence window — and
+    # return the outflow entries (the occurrence-date carrier, consistent with
+    # create_from_transfer).
+    def transfer_matching_transactions
+      return Entry.none unless account && destination_account
+
+      outflow_entries = day_of_month_scope(
+        amount_window_scope(account.entries.where(entryable_type: "Transaction").where(currency: currency))
+      ).order(date: :desc)
+
+      paired_outflow_transaction_ids = Transfer
+        .where(outflow_transaction_id: outflow_entries.select(:entryable_id))
+        .where(inflow_transaction_id:
+          destination_account.entries.where(entryable_type: "Transaction").select(:entryable_id))
+        .pluck(:outflow_transaction_id)
+
+      outflow_entries.where(entryable_id: paired_outflow_transaction_ids)
+    end
+
+    # Transaction entries whose amount fits the pattern: exact, or within the
+    # configured variance band for manual recurring rows.
+    def amount_window_scope(relation)
+      if manual? && has_amount_variance?
+        relation.where("entries.amount BETWEEN ? AND ?", expected_amount_min, expected_amount_max)
+      else
+        relation.where("entries.amount = ?", amount)
+      end
+    end
+
+    # Entries whose day-of-month lands within ±2 days of the expected day.
+    def day_of_month_scope(relation)
+      relation.where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?",
+                     [ expected_day_of_month - 2, 1 ].max,
+                     [ expected_day_of_month + 2, 31 ].min)
+    end
+
     def monetizable_currency
       currency
     end

app/models/recurring_transaction/cleaner.rb

@@ -9,18 +9,15 @@ class RecurringTransaction
     # Mark recurring transactions as inactive if they haven't occurred recently
     # Uses 2 months for automatic recurring, 6 months for manual recurring.
     #
-    # Transfer rows (destination_account_id present) are skipped: their
-    # `matching_transactions` helper looks at single-account name/amount
-    # which never matches a Transfer pair, so the Cleaner would
-    # incorrectly mark a still-recurring transfer inactive at the
-    # 6-month threshold. Issue #1590 tracks pair-detection-aware
-    # matching for recurring transfers.
+    # Transfer rows (destination_account_id present) are included: as of issue
+    # #1590, `matching_transactions` detects the Transfer pair, so a still-active
+    # transfer keeps surfacing recent matches and stays active, while one whose
+    # pair has genuinely stopped is correctly retired.
     def cleanup_stale_transactions
       stale_count = 0
 
       family.recurring_transactions
             .active
-            .where(destination_account_id: nil)
             .find_each do |recurring_transaction|
         next unless recurring_transaction.should_be_inactive?
 

test/models/recurring_transaction_test.rb

@@ -998,12 +998,9 @@ class RecurringTransactionTest < ActiveSupport::TestCase
     assert_not RecurringTransaction.exists?(rt.id)
   end
 
-  test "Cleaner skips recurring transfers so they aren't mistakenly marked inactive" do
-    # `matching_transactions` is single-account name/amount-based and never
-    # matches a Transfer pair, so without the skip the recurring transfer
-    # would flip to inactive at the 6-month threshold even when the user
-    # is still doing the transfer monthly. Issue #1590 tracks the proper
-    # pair-detection fix.
+  test "Cleaner keeps a recurring transfer active when its pair still occurs (issue #1590)" do
+    # The seed name rarely matches future occurrences, so pair detection (not
+    # name matching) is what keeps a live transfer active past the threshold.
     rt = @family.recurring_transactions.create!(
       account: @account, destination_account: accounts(:credit_card),
       name: "Transfer to CC", amount: 250, currency: "USD",
@@ -1012,12 +1009,61 @@ class RecurringTransactionTest < ActiveSupport::TestCase
       next_expected_date: 5.days.from_now.to_date,
       manual: true
     )
-    assert rt.should_be_inactive?, "guard sanity: row would be marked inactive without the skip"
+    assert rt.should_be_inactive?, "guard sanity: stale last_occurrence_date"
+
+    # A fresh transfer pair this cycle, carrying a *different* free-text name.
+    date = 1.month.ago.beginning_of_month + 4.days # day-of-month 5
+    outflow = @account.entries.create!(
+      date: date, amount: 250, currency: "USD", name: "rent transfer",
+      entryable: Transaction.new(kind: "funds_movement")
+    )
+    inflow = accounts(:credit_card).entries.create!(
+      date: date, amount: -250, currency: "USD", name: "rent transfer",
+      entryable: Transaction.new(kind: "funds_movement")
+    )
+    Transfer.create!(outflow_transaction: outflow.entryable, inflow_transaction: inflow.entryable)
 
     RecurringTransaction.cleanup_stale_for(@family)
     assert_equal "active", rt.reload.status
   end
 
+  test "Cleaner retires a recurring transfer whose pair has stopped" do
+    # No matching Transfer pair → genuinely stale → should be retired. This is
+    # the correctness the pair-detection (vs the old blanket skip) buys us.
+    rt = @family.recurring_transactions.create!(
+      account: @account, destination_account: accounts(:credit_card),
+      name: "Transfer to CC", amount: 250, currency: "USD",
+      expected_day_of_month: 5,
+      last_occurrence_date: 7.months.ago.to_date,
+      next_expected_date: 5.days.from_now.to_date,
+      manual: true
+    )
+
+    RecurringTransaction.cleanup_stale_for(@family)
+    assert_equal "inactive", rt.reload.status
+  end
+
+  test "matching_transactions finds the transfer pair regardless of occurrence name" do
+    rt = @family.recurring_transactions.create!(
+      account: @account, destination_account: accounts(:credit_card),
+      name: "Transfer to CC", amount: 250, currency: "USD",
+      expected_day_of_month: 5, last_occurrence_date: Date.current,
+      next_expected_date: 1.month.from_now.to_date, manual: true
+    )
+    date = Date.current.beginning_of_month + 4.days # day-of-month 5
+    outflow = @account.entries.create!(
+      date: date, amount: 250, currency: "USD", name: "an importer's wording",
+      entryable: Transaction.new(kind: "funds_movement")
+    )
+    inflow = accounts(:credit_card).entries.create!(
+      date: date, amount: -250, currency: "USD", name: "an importer's wording",
+      entryable: Transaction.new(kind: "funds_movement")
+    )
+    Transfer.create!(outflow_transaction: outflow.entryable, inflow_transaction: inflow.entryable)
+
+    assert_includes rt.matching_transactions.map(&:id), outflow.id
+  end
+
   test "Identifier#update_manual_recurring_transactions skips recurring transfers" do
     # Same reasoning as the Cleaner skip. Without the guard, the helper
     # would call find_matching_transaction_entries (single-account, by