Fix: SimpleFIN account re-link duplication (#554)

* Add orphan pruning tests for Simplefin importer and implement pruning logic

- Introduced `SimplefinItem::ImporterOrphanPruneTest` to verify orphaned `SimplefinAccount` pruning scenarios.
- Added logic in `SimplefinItem::Importer` to remove orphaned `SimplefinAccounts` when upstream account IDs change.
- Ensured linked accounts via legacy FK or `AccountProvider` are preserved during pruning.
- Updated sync stats to track pruned accounts.

* Optimize SimplefinAccount query in importer to prevent N+1 issues

- Added eager-loading of `account` and `account_provider` associations when retrieving orphaned `SimplefinAccounts`.

---------

Co-authored-by: Josh Waldrep <joshua.waldrep5+github@gmail.com>

app/models/simplefin_item/importer.rb

@@ -402,9 +402,48 @@ class SimplefinItem::Importer
             persist_stats!
           end
         end
+
+        # Clean up orphaned SimplefinAccount records whose account_id no longer exists upstream.
+        # This handles the case where a user deletes and re-adds an institution in SimpleFIN,
+        # which generates new account IDs. Without this cleanup, both old (stale) and new
+        # SimplefinAccount records would appear in the setup UI as duplicates.
+        upstream_account_ids = discovery_data[:accounts].map { |a| a[:id].to_s }.compact
+        prune_orphaned_simplefin_accounts(upstream_account_ids)
       end
     end
 
+    # Removes SimplefinAccount records that no longer exist upstream and are not linked to any Account.
+    # This prevents duplicate accounts from appearing in the setup UI after a user re-adds an
+    # institution in SimpleFIN (which generates new account IDs).
+    def prune_orphaned_simplefin_accounts(upstream_account_ids)
+      return if upstream_account_ids.blank?
+
+      # Find SimplefinAccount records with account_ids NOT in the upstream set
+      # Eager-load associations to prevent N+1 queries when checking linkage
+      orphaned = simplefin_item.simplefin_accounts
+        .includes(:account, :account_provider)
+        .where.not(account_id: upstream_account_ids)
+        .where.not(account_id: nil)
+
+      orphaned.each do |sfa|
+        # Only delete if not linked to any Account (via legacy FK or AccountProvider)
+        # Note: sfa.account checks the legacy FK on Account.simplefin_account_id
+        #       sfa.account_provider checks the new AccountProvider join table
+        linked_via_legacy = sfa.account.present?
+        linked_via_provider = sfa.account_provider.present?
+
+        if !linked_via_legacy && !linked_via_provider
+          Rails.logger.info "SimpleFin: Pruning orphaned SimplefinAccount id=#{sfa.id} account_id=#{sfa.account_id} (no longer exists upstream)"
+          stats["accounts_pruned"] = stats.fetch("accounts_pruned", 0) + 1
+          sfa.destroy
+        else
+          Rails.logger.info "SimpleFin: Keeping stale SimplefinAccount id=#{sfa.id} account_id=#{sfa.account_id} (still linked to Account)"
+        end
+      end
+
+      persist_stats! if stats["accounts_pruned"].to_i > 0
+    end
+
     # Fetches accounts (and optionally transactions/holdings) from SimpleFin.
     #
     # Params:

test/models/simplefin_item/importer_orphan_prune_test.rb

@@ -0,0 +1,174 @@
+require "test_helper"
+
+class SimplefinItem::ImporterOrphanPruneTest < ActiveSupport::TestCase
+  setup do
+    @family = families(:dylan_family)
+    @item = SimplefinItem.create!(family: @family, name: "SF Conn", access_url: "https://example.com/access")
+    @sync = Sync.create!(syncable: @item)
+  end
+
+  test "prunes orphaned SimplefinAccount records when upstream account_ids change" do
+    # Create an existing SimplefinAccount with an OLD account_id (simulating a previously synced account)
+    old_sfa = SimplefinAccount.create!(
+      simplefin_item: @item,
+      account_id: "ACT-old-id-12345",
+      name: "Business",
+      currency: "USD",
+      current_balance: 100,
+      account_type: "checking"
+    )
+
+    # Stub provider to return accounts with NEW account_ids (simulating re-added institution)
+    mock_provider = mock()
+    mock_provider.expects(:get_accounts).at_least_once.returns({
+      accounts: [
+        { id: "ACT-new-id-67890", name: "Business", balance: "288.41", currency: "USD", type: "checking" }
+      ]
+    })
+
+    importer = SimplefinItem::Importer.new(@item, simplefin_provider: mock_provider, sync: @sync)
+    importer.send(:perform_account_discovery)
+
+    # The old SimplefinAccount should be pruned
+    assert_nil SimplefinAccount.find_by(id: old_sfa.id), "old SimplefinAccount with stale account_id should be deleted"
+
+    # A new SimplefinAccount should exist with the new account_id
+    new_sfa = @item.simplefin_accounts.find_by(account_id: "ACT-new-id-67890")
+    assert_not_nil new_sfa, "new SimplefinAccount should be created"
+    assert_equal "Business", new_sfa.name
+
+    # Stats should reflect the pruning
+    stats = @sync.reload.sync_stats
+    assert_equal 1, stats["accounts_pruned"], "should track pruned accounts"
+  end
+
+  test "does not prune SimplefinAccount that is linked to an Account via legacy FK" do
+    # Create a SimplefinAccount with an old account_id
+    old_sfa = SimplefinAccount.create!(
+      simplefin_item: @item,
+      account_id: "ACT-old-id-12345",
+      name: "Business",
+      currency: "USD",
+      current_balance: 100,
+      account_type: "checking"
+    )
+
+    # Link it to an Account via legacy FK
+    account = Account.create!(
+      family: @family,
+      name: "Business Checking",
+      currency: "USD",
+      balance: 100,
+      accountable: Depository.create!(subtype: :checking),
+      simplefin_account_id: old_sfa.id
+    )
+
+    # Stub provider to return accounts with NEW account_ids
+    mock_provider = mock()
+    mock_provider.expects(:get_accounts).at_least_once.returns({
+      accounts: [
+        { id: "ACT-new-id-67890", name: "Business", balance: "288.41", currency: "USD", type: "checking" }
+      ]
+    })
+
+    importer = SimplefinItem::Importer.new(@item, simplefin_provider: mock_provider, sync: @sync)
+    importer.send(:perform_account_discovery)
+
+    # The old SimplefinAccount should NOT be pruned because it's linked
+    assert_not_nil SimplefinAccount.find_by(id: old_sfa.id), "linked SimplefinAccount should not be deleted"
+
+    # New SimplefinAccount should also exist
+    new_sfa = @item.simplefin_accounts.find_by(account_id: "ACT-new-id-67890")
+    assert_not_nil new_sfa, "new SimplefinAccount should be created"
+
+    # Stats should not show any pruning
+    stats = @sync.reload.sync_stats
+    assert_nil stats["accounts_pruned"], "should not prune linked accounts"
+  end
+
+  test "does not prune SimplefinAccount that is linked via AccountProvider" do
+    # Create a SimplefinAccount with an old account_id
+    old_sfa = SimplefinAccount.create!(
+      simplefin_item: @item,
+      account_id: "ACT-old-id-12345",
+      name: "Business",
+      currency: "USD",
+      current_balance: 100,
+      account_type: "checking"
+    )
+
+    # Create an Account and link via AccountProvider (new system)
+    account = Account.create!(
+      family: @family,
+      name: "Business Checking",
+      currency: "USD",
+      balance: 100,
+      accountable: Depository.create!(subtype: :checking)
+    )
+    AccountProvider.create!(account: account, provider: old_sfa)
+
+    # Stub provider to return accounts with NEW account_ids
+    mock_provider = mock()
+    mock_provider.expects(:get_accounts).at_least_once.returns({
+      accounts: [
+        { id: "ACT-new-id-67890", name: "Business", balance: "288.41", currency: "USD", type: "checking" }
+      ]
+    })
+
+    importer = SimplefinItem::Importer.new(@item, simplefin_provider: mock_provider, sync: @sync)
+    importer.send(:perform_account_discovery)
+
+    # The old SimplefinAccount should NOT be pruned because it's linked via AccountProvider
+    assert_not_nil SimplefinAccount.find_by(id: old_sfa.id), "linked SimplefinAccount should not be deleted"
+
+    # Stats should not show any pruning
+    stats = @sync.reload.sync_stats
+    assert_nil stats["accounts_pruned"], "should not prune linked accounts"
+  end
+
+  test "prunes multiple orphaned SimplefinAccounts when institution re-added with all new IDs" do
+    # Create two old SimplefinAccounts (simulating two accounts from before re-add)
+    old_sfa1 = SimplefinAccount.create!(
+      simplefin_item: @item,
+      account_id: "ACT-old-business",
+      name: "Business",
+      currency: "USD",
+      current_balance: 28.41,
+      account_type: "checking"
+    )
+
+    old_sfa2 = SimplefinAccount.create!(
+      simplefin_item: @item,
+      account_id: "ACT-old-personal",
+      name: "Personal",
+      currency: "USD",
+      current_balance: 308.43,
+      account_type: "checking"
+    )
+
+    # Stub provider to return accounts with entirely NEW account_ids
+    mock_provider = mock()
+    mock_provider.expects(:get_accounts).at_least_once.returns({
+      accounts: [
+        { id: "ACT-new-business", name: "Business", balance: "288.41", currency: "USD", type: "checking" },
+        { id: "ACT-new-personal", name: "Personal", balance: "22.43", currency: "USD", type: "checking" }
+      ]
+    })
+
+    importer = SimplefinItem::Importer.new(@item, simplefin_provider: mock_provider, sync: @sync)
+    importer.send(:perform_account_discovery)
+
+    # Both old SimplefinAccounts should be pruned
+    assert_nil SimplefinAccount.find_by(id: old_sfa1.id), "old Business SimplefinAccount should be deleted"
+    assert_nil SimplefinAccount.find_by(id: old_sfa2.id), "old Personal SimplefinAccount should be deleted"
+
+    # New SimplefinAccounts should exist
+    assert_equal 2, @item.simplefin_accounts.reload.count, "should have exactly 2 SimplefinAccounts"
+    assert_not_nil @item.simplefin_accounts.find_by(account_id: "ACT-new-business")
+    assert_not_nil @item.simplefin_accounts.find_by(account_id: "ACT-new-personal")
+
+    # Stats should reflect both pruned
+    stats = @sync.reload.sync_stats
+    assert_equal 2, stats["accounts_pruned"], "should track both pruned accounts"
+  end
+end