Harden SimpleFin sync: retries, safer imports, manual relinking, and data-quality reconciliation (#544)

* Add tests and enhance logic for SimpleFin account synchronization and reconciliation - Added retry logic with exponential backoff for network errors in `Provider::Simplefin`. - Introduced tests to verify retry functionality and error handling for rate-limit, server errors, and stale data. - Updated `SimplefinItem` to detect stale sync status and reconciliation issues. - Enhanced UI to display stale sync warnings and data integrity notices. - Improved SimpleFin account matching during updates with multi-tier strategy (ID, fingerprint, fuzzy match). - Added transaction reconciliation logic to detect data gaps, transaction count drops, and duplicate transaction IDs. * Introduce `SimplefinConnectionUpdateJob` for asynchronous SimpleFin connection updates - Moved SimpleFin connection update logic to `SimplefinConnectionUpdateJob` to improve response times by offloading network retries, data fetching, and reconciliation tasks. - Enhanced SimpleFin account matching with a multi-tier strategy (ID, fingerprint, fuzzy name match). - Added retry logic and bounded latency for token claim requests in `Provider::Simplefin`. - Updated tests to cover the new job flow and ensure correct account reconciliation during updates. * Remove unused SimpleFin account matching logic and improve error handling in `SimplefinConnectionUpdateJob` - Deleted the multi-tier account matching logic from `SimplefinItemsController` as it is no longer used. - Enhanced error handling in `SimplefinConnectionUpdateJob` to gracefully handle import failures, ensuring orphaned items can be manually resolved. - Updated job flow to conditionally set item status based on the success of import operations. * Fix SimpleFin sync: check both legacy FK and AccountProvider for linked accounts * Add crypto, checking, savings, and cash account detection; refine subtype selection and linking - Enhanced `Simplefin::AccountTypeMapper` to include detection for crypto, checking, savings, and standalone cash accounts. - Improved subtype selection UI with validation and warning indicators for missing selections. - Updated SimpleFin account linking to handle both legacy FK and `AccountProvider` associations consistently. - Refined job flow and importer logic for better handling of linked accounts and subtype inference. * Improve `SimplefinConnectionUpdateJob` and holdings processing logic - Fixed race condition in `SimplefinConnectionUpdateJob` by moving `destroy_later` calls outside of transactions. - Updated fuzzy name match logic to use Levenshtein distance for better accuracy. - Enhanced synthetic ticker generation in holdings processor with hash suffix for uniqueness. * Refine SimpleFin entry processing logic and ensure `extra` data persistence - Simplified pending flag determination to rely solely on provider-supplied values. - Fixed potential stale values in `extra` by ensuring deep merge overwrite with `entry.transaction.save!`. * Replace hardcoded fallback transaction description with localized string * Refine pending flag logic in SimpleFin processor tests - Adjust test to prevent falsely inferring pending status from missing posted dates. - Ensure provider explicitly sets pending flag for transactions. * Add `has_many :holdings` association to `AccountProvider` with `dependent: :nullify` --------- Co-authored-by: Josh Waldrep <joshua.waldrep5+github@gmail.com>
2026-04-19 03:54:08 +00:00 · 2026-01-05 16:11:47 -05:00
parent b3330a318d
commit c12c585a0e
21 changed files with 913 additions and 179 deletions
--- a/test/controllers/simplefin_items_controller_test.rb
+++ b/test/controllers/simplefin_items_controller_test.rb
@@ -1,6 +1,7 @@
 require "test_helper"

 class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
+  include ActiveJob::TestHelper
  fixtures :users, :families
  setup do
    sign_in users(:family_admin)
@@ -154,22 +155,20 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
  test "should update simplefin item with valid token" do
    @simplefin_item.update!(status: :requires_update)

-    # Mock the SimpleFin provider to prevent real API calls
-    mock_provider = mock()
-    mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access")
-    mock_provider.expects(:get_accounts).returns({ accounts: [] }).at_least_once
-    Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once
+    token = Base64.strict_encode64("https://example.com/claim")

-    # Let the real create_simplefin_item! method run - don't mock it
+    SimplefinConnectionUpdateJob.expects(:perform_later).with(
+      family_id: @family.id,
+      old_simplefin_item_id: @simplefin_item.id,
+      setup_token: token
+    ).once

    patch simplefin_item_url(@simplefin_item), params: {
-      simplefin_item: { setup_token: "valid_token" }
+      simplefin_item: { setup_token: token }
    }

    assert_redirected_to accounts_path
    assert_equal "SimpleFin connection updated.", flash[:notice]
-    @simplefin_item.reload
-    assert @simplefin_item.scheduled_for_deletion?
  end

  test "should handle update with invalid token" do
@@ -186,6 +185,8 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
  test "should transfer accounts when updating simplefin item token" do
    @simplefin_item.update!(status: :requires_update)

+    token = Base64.strict_encode64("https://example.com/claim")
+
    # Create old SimpleFin accounts linked to Maybe accounts
    old_simplefin_account1 = @simplefin_item.simplefin_accounts.create!(
      name: "Test Checking",
@@ -228,7 +229,7 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest

    # Mock only the external API calls, let business logic run
    mock_provider = mock()
-    mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access")
+    mock_provider.expects(:claim_access_url).with(token).returns("https://example.com/new_access")
    mock_provider.expects(:get_accounts).returns({
      accounts: [
        {
@@ -251,10 +252,13 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
    }).at_least_once
    Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once

-    # Perform the update
-    patch simplefin_item_url(@simplefin_item), params: {
-      simplefin_item: { setup_token: "valid_token" }
-    }
+    # Perform the update (async job), but execute enqueued jobs inline so we can
+    # assert the link transfers.
+    perform_enqueued_jobs(only: SimplefinConnectionUpdateJob) do
+      patch simplefin_item_url(@simplefin_item), params: {
+        simplefin_item: { setup_token: token }
+      }
+    end

    assert_redirected_to accounts_path
    assert_equal "SimpleFin connection updated.", flash[:notice]
@@ -279,11 +283,7 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
    assert_equal new_sf_account1.id, maybe_account1.simplefin_account_id
    assert_equal new_sf_account2.id, maybe_account2.simplefin_account_id

-    # Verify old SimpleFin accounts no longer reference Maybe accounts
-    old_simplefin_account1.reload
-    old_simplefin_account2.reload
-    assert_nil old_simplefin_account1.current_account
-    assert_nil old_simplefin_account2.current_account
+    # The old item will be deleted asynchronously; until then, legacy links should be moved.

    # Verify old SimpleFin item is scheduled for deletion
    @simplefin_item.reload
@@ -293,6 +293,8 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
  test "should handle partial account matching during token update" do
    @simplefin_item.update!(status: :requires_update)

+    token = Base64.strict_encode64("https://example.com/claim")
+
    # Create old SimpleFin account
    old_simplefin_account = @simplefin_item.simplefin_accounts.create!(
      name: "Test Checking",
@@ -316,19 +318,19 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest

    # Mock only the external API calls, let business logic run
    mock_provider = mock()
-    mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access")
+    mock_provider.expects(:claim_access_url).with(token).returns("https://example.com/new_access")
    # Return empty accounts list to simulate account was removed from bank
    mock_provider.expects(:get_accounts).returns({ accounts: [] }).at_least_once
    Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once

    # Perform update
-    patch simplefin_item_url(@simplefin_item), params: {
-      simplefin_item: { setup_token: "valid_token" }
-    }
+    perform_enqueued_jobs(only: SimplefinConnectionUpdateJob) do
+      patch simplefin_item_url(@simplefin_item), params: {
+        simplefin_item: { setup_token: token }
+      }
+    end

-    assert_response :redirect
-    uri2 = URI(response.redirect_url)
-    assert_equal "/accounts", uri2.path
+    assert_redirected_to accounts_path

    # Verify Maybe account still linked to old SimpleFin account (no transfer occurred)
    maybe_account.reload
@@ -450,30 +452,27 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
  test "update redirects to accounts after setup without forcing a modal" do
    @simplefin_item.update!(status: :requires_update)

-    # Mock provider to return one account so updated_item creates SFAs
-    mock_provider = mock()
-    mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access")
-    mock_provider.expects(:get_accounts).returns({
-      accounts: [
-        { id: "sf_auto_open_1", name: "Auto Open Checking", type: "depository", currency: "USD", balance: 100, transactions: [] }
-      ]
-    }).at_least_once
-    Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once
+    token = Base64.strict_encode64("https://example.com/claim")

-    patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: "valid_token" } }
+    SimplefinConnectionUpdateJob.expects(:perform_later).with(
+      family_id: @family.id,
+      old_simplefin_item_id: @simplefin_item.id,
+      setup_token: token
+    ).once

-    assert_response :redirect
-    uri = URI(response.redirect_url)
-    assert_equal "/accounts", uri.path
+    patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: token } }
+
+    assert_redirected_to accounts_path
  end

  test "create does not auto-open when no candidates or unlinked" do
    # Mock provider interactions for item creation (no immediate account import on create)
    mock_provider = mock()
-    mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access")
+    token = Base64.strict_encode64("https://example.com/claim")
+    mock_provider.expects(:claim_access_url).with(token).returns("https://example.com/new_access")
    Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once

-    post simplefin_items_url, params: { simplefin_item: { setup_token: "valid_token" } }
+    post simplefin_items_url, params: { simplefin_item: { setup_token: token } }

    assert_response :redirect
    uri = URI(response.redirect_url)
@@ -485,12 +484,15 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
  test "update does not auto-open when no SFAs present" do
    @simplefin_item.update!(status: :requires_update)

-    mock_provider = mock()
-    mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access")
-    mock_provider.expects(:get_accounts).returns({ accounts: [] }).at_least_once
-    Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once
+    token = Base64.strict_encode64("https://example.com/claim")

-    patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: "valid_token" } }
+    SimplefinConnectionUpdateJob.expects(:perform_later).with(
+      family_id: @family.id,
+      old_simplefin_item_id: @simplefin_item.id,
+      setup_token: token
+    ).once
+
+    patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: token } }

    assert_response :redirect
    uri = URI(response.redirect_url)
--- a/test/models/provider/simplefin_test.rb
+++ b/test/models/provider/simplefin_test.rb
@@ -0,0 +1,144 @@
+require "test_helper"
+
+class Provider::SimplefinTest < ActiveSupport::TestCase
+  setup do
+    @provider = Provider::Simplefin.new
+    @access_url = "https://example.com/simplefin/access"
+  end
+
+  test "retries on Net::ReadTimeout and succeeds on retry" do
+    # First call raises timeout, second call succeeds
+    mock_response = OpenStruct.new(code: 200, body: '{"accounts": []}')
+
+    HTTParty.expects(:get)
+      .times(2)
+      .raises(Net::ReadTimeout.new("Connection timed out"))
+      .then.returns(mock_response)
+
+    # Stub sleep to avoid actual delays in tests
+    @provider.stubs(:sleep)
+
+    result = @provider.get_accounts(@access_url)
+    assert_equal({ accounts: [] }, result)
+  end
+
+  test "retries on Net::OpenTimeout and succeeds on retry" do
+    mock_response = OpenStruct.new(code: 200, body: '{"accounts": []}')
+
+    HTTParty.expects(:get)
+      .times(2)
+      .raises(Net::OpenTimeout.new("Connection timed out"))
+      .then.returns(mock_response)
+
+    @provider.stubs(:sleep)
+
+    result = @provider.get_accounts(@access_url)
+    assert_equal({ accounts: [] }, result)
+  end
+
+  test "retries on SocketError and succeeds on retry" do
+    mock_response = OpenStruct.new(code: 200, body: '{"accounts": []}')
+
+    HTTParty.expects(:get)
+      .times(2)
+      .raises(SocketError.new("Failed to open TCP connection"))
+      .then.returns(mock_response)
+
+    @provider.stubs(:sleep)
+
+    result = @provider.get_accounts(@access_url)
+    assert_equal({ accounts: [] }, result)
+  end
+
+  test "raises SimplefinError after max retries exceeded" do
+    HTTParty.expects(:get)
+      .times(4) # Initial + 3 retries
+      .raises(Net::ReadTimeout.new("Connection timed out"))
+
+    @provider.stubs(:sleep)
+
+    error = assert_raises(Provider::Simplefin::SimplefinError) do
+      @provider.get_accounts(@access_url)
+    end
+
+    assert_equal :network_error, error.error_type
+    assert_match(/Network error after 3 retries/, error.message)
+  end
+
+  test "does not retry on non-retryable errors" do
+    HTTParty.expects(:get)
+      .times(1)
+      .raises(ArgumentError.new("Invalid argument"))
+
+    error = assert_raises(Provider::Simplefin::SimplefinError) do
+      @provider.get_accounts(@access_url)
+    end
+
+    assert_equal :request_failed, error.error_type
+  end
+
+  test "handles HTTP 429 rate limit response" do
+    mock_response = OpenStruct.new(code: 429, body: "Rate limit exceeded")
+
+    HTTParty.expects(:get).returns(mock_response)
+
+    error = assert_raises(Provider::Simplefin::SimplefinError) do
+      @provider.get_accounts(@access_url)
+    end
+
+    assert_equal :rate_limited, error.error_type
+    assert_match(/rate limit exceeded/i, error.message)
+  end
+
+  test "handles HTTP 500 server error response" do
+    mock_response = OpenStruct.new(code: 500, body: "Internal Server Error")
+
+    HTTParty.expects(:get).returns(mock_response)
+
+    error = assert_raises(Provider::Simplefin::SimplefinError) do
+      @provider.get_accounts(@access_url)
+    end
+
+    assert_equal :server_error, error.error_type
+  end
+
+  test "claim_access_url retries on network errors" do
+    setup_token = Base64.encode64("https://example.com/claim")
+    mock_response = OpenStruct.new(code: 200, body: "https://example.com/access")
+
+    HTTParty.expects(:post)
+      .times(2)
+      .raises(Net::ReadTimeout.new("Connection timed out"))
+      .then.returns(mock_response)
+
+    @provider.stubs(:sleep)
+
+    result = @provider.claim_access_url(setup_token)
+    assert_equal "https://example.com/access", result
+  end
+
+  test "exponential backoff delay increases with retries" do
+    provider = Provider::Simplefin.new
+
+    # Access private method for testing
+    delay1 = provider.send(:calculate_retry_delay, 1)
+    delay2 = provider.send(:calculate_retry_delay, 2)
+    delay3 = provider.send(:calculate_retry_delay, 3)
+
+    # Delays should increase (accounting for jitter)
+    # Base delays: 2, 4, 8 seconds (with up to 25% jitter)
+    assert delay1 >= 2 && delay1 <= 2.5, "First retry delay should be ~2s"
+    assert delay2 >= 4 && delay2 <= 5, "Second retry delay should be ~4s"
+    assert delay3 >= 8 && delay3 <= 10, "Third retry delay should be ~8s"
+  end
+
+  test "retry delay is capped at MAX_RETRY_DELAY" do
+    provider = Provider::Simplefin.new
+
+    # Test with a high retry count that would exceed max delay
+    delay = provider.send(:calculate_retry_delay, 10)
+
+    assert delay <= Provider::Simplefin::MAX_RETRY_DELAY,
+      "Delay should be capped at MAX_RETRY_DELAY (#{Provider::Simplefin::MAX_RETRY_DELAY}s)"
+  end
+end
--- a/test/models/simplefin_entry/processor_test.rb
+++ b/test/models/simplefin_entry/processor_test.rb
@@ -53,7 +53,9 @@ class SimplefinEntry::ProcessorTest < ActiveSupport::TestCase
    assert_equal "Order #1234", sf["description"]
    assert_equal({ "category" => "restaurants", "check_number" => nil }, sf["extra"])
  end
-  test "flags pending transaction when posted is nil and transacted_at present" do
+  test "does not flag pending when posted is nil but provider pending flag not set" do
+    # Previously we inferred pending from missing posted date, but this was too aggressive -
+    # some providers don't supply posted dates even for settled transactions
    tx = {
      id: "tx_pending_1",
      amount: "-20.00",
@@ -70,7 +72,7 @@ class SimplefinEntry::ProcessorTest < ActiveSupport::TestCase
    entry = @account.entries.find_by!(external_id: "simplefin_tx_pending_1", source: "simplefin")
    sf = entry.transaction.extra.fetch("simplefin")

-    assert_equal true, sf["pending"], "expected pending flag to be true"
+    assert_equal false, sf["pending"], "expected pending flag to be false when provider doesn't explicitly set pending"
  end

  test "captures FX metadata when tx currency differs from account currency" do