Correct brokerage cash calculation for SimpleFIN investment accounts (#710)

* Refactor: Enhance cash balance calculation and holdings management with money market classification and provider-sourced data handling

* Fix: Clear fixture holdings in test to ensure clean creation and update raw_holdings_payload format

---------

Co-authored-by: luckyPipewrench <luckypipewrench@proton.me>

app/models/holding/materializer.rb

@@ -16,6 +16,14 @@ class Holding::Materializer
       purge_stale_holdings
     end
 
+    # Clean up calculated holdings for securities that now have provider-sourced holdings
+    # This prevents duplicates when a manually-entered account gets linked to a provider
+    cleanup_calculated_holdings_for_provider_securities
+
+    # Reload holdings association to clear any cached stale data
+    # This ensures subsequent Balance calculations see the fresh holdings
+    account.holdings.reload
+
     @holdings
   end
 
@@ -39,6 +47,9 @@ class Holding::Materializer
       holdings_to_upsert_without_cost = []
 
       @holdings.each do |holding|
+        # Skip securities that have provider-sourced holdings - don't overwrite provider data
+        next if provider_sourced_security_ids.include?(holding.security_id)
+
         key = holding_key(holding)
         existing = existing_holdings_map[key]
 
@@ -88,12 +99,37 @@ class Holding::Materializer
       # Load holdings that might affect reconciliation:
       # - Locked holdings (must preserve their cost_basis)
       # - Holdings with a source (need to check priority)
+      # - Provider-sourced holdings (must not be overwritten)
       account.holdings
         .where(cost_basis_locked: true)
         .or(account.holdings.where.not(cost_basis_source: nil))
+        .or(account.holdings.where.not(account_provider_id: nil))
         .index_by { |h| holding_key(h) }
     end
 
+    # Get security IDs that have provider-sourced holdings (any date)
+    # These should be preserved and not overwritten by calculated holdings
+    def provider_sourced_security_ids
+      @provider_sourced_security_ids ||= account.holdings
+        .where.not(account_provider_id: nil)
+        .distinct
+        .pluck(:security_id)
+    end
+
+    # Remove calculated holdings (account_provider_id IS NULL) for securities
+    # that now have provider-sourced holdings. This prevents duplicates when
+    # a manually-entered account gets linked to a provider.
+    def cleanup_calculated_holdings_for_provider_securities
+      return if provider_sourced_security_ids.empty?
+
+      deleted_count = account.holdings
+        .where(account_provider_id: nil)
+        .where(security_id: provider_sourced_security_ids)
+        .delete_all
+
+      Rails.logger.info("Cleaned up #{deleted_count} calculated holdings for provider-sourced securities") if deleted_count > 0
+    end
+
     def holding_key(holding)
       [ holding.account_id || account.id, holding.security_id, holding.date, holding.currency ]
     end
@@ -101,12 +137,16 @@ class Holding::Materializer
     def purge_stale_holdings
       portfolio_security_ids = account.entries.trades.map { |entry| entry.entryable.security_id }.uniq
 
-      # If there are no securities in the portfolio, delete all holdings
+      # Never delete provider-sourced holdings - they're authoritative from the provider
+      # If there are no securities in the portfolio, only delete non-provider holdings
       if portfolio_security_ids.empty?
-        Rails.logger.info("Clearing all holdings (no securities)")
-        account.holdings.delete_all
+        Rails.logger.info("Clearing non-provider holdings (no securities from trades)")
+        account.holdings.where(account_provider_id: nil).delete_all
       else
-        deleted_count = account.holdings.delete_by("date < ? OR security_id NOT IN (?)", account.start_date, portfolio_security_ids)
+        # Keep provider holdings and holdings for known securities within date range
+        deleted_count = account.holdings
+          .where(account_provider_id: nil)
+          .delete_by("date < ? OR security_id NOT IN (?)", account.start_date, portfolio_security_ids)
         Rails.logger.info("Purged #{deleted_count} stale holdings") if deleted_count > 0
       end
     end

app/models/simplefin_account.rb

@@ -23,12 +23,17 @@ class SimplefinAccount < ApplicationRecord
     acct = current_account
     return nil unless acct
 
-    AccountProvider
+    provider = AccountProvider
       .find_or_initialize_by(provider_type: "SimplefinAccount", provider_id: id)
-      .tap do |provider|
-        provider.account = acct
-        provider.save!
+      .tap do |p|
+        p.account = acct
+        p.save!
       end
+
+    # Reload the association so future accesses don't return stale/nil value
+    reload_account_provider
+
+    provider
   rescue => e
     Rails.logger.warn("SimplefinAccount##{id}: failed to ensure AccountProvider link: #{e.class} - #{e.message}")
     nil

app/models/simplefin_account/investments/balance_calculator.rb

@@ -1,6 +1,22 @@
 # SimpleFin Investment balance calculator
 # SimpleFin provides clear balance and holdings data, so calculations are simpler than Plaid
 class SimplefinAccount::Investments::BalanceCalculator
+  # Common money market fund tickers that should be treated as cash equivalents
+  # These are settlement funds that users consider "cash available to invest"
+  MONEY_MARKET_TICKERS = %w[
+    VMFXX VMMXX VMRXX VUSXX
+    SPAXX FDRXX SPRXX FZFXX FDLXX
+    SWVXX SNVXX SNOXX
+    TTTXX PRTXX
+  ].freeze
+
+  # Patterns that indicate money market funds (case-insensitive)
+  MONEY_MARKET_PATTERNS = [
+    /money\s*market/i,
+    /settlement\s*fund/i,
+    /cash\s*reserve/i
+  ].freeze
+
   def initialize(simplefin_account)
     @simplefin_account = simplefin_account
   end
@@ -11,39 +27,64 @@ class SimplefinAccount::Investments::BalanceCalculator
   end
 
   def cash_balance
-    # Calculate cash balance as total balance minus holdings value
+    # Calculate cash balance as total balance minus non-cash holdings value
+    # Money market funds are treated as cash equivalents (settlement funds)
     total_balance = balance
-    holdings_value = total_holdings_value
+    non_cash_value = non_cash_holdings_value
 
-    cash = total_balance - holdings_value
+    cash = total_balance - non_cash_value
 
-    # Ensure non-negative cash balance
-    [ cash, BigDecimal("0") ].max
+    # Allow negative cash to represent margin debt (matching Plaid's approach)
+    # Log a warning for debugging, but don't clamp to zero
+    if cash.negative?
+      Rails.logger.info("SimpleFin: negative cash_balance (#{cash}) for account #{simplefin_account.account_id || simplefin_account.id} - may indicate margin usage or stale data")
+    end
+
+    cash
   end
 
   private
     attr_reader :simplefin_account
 
-    def total_holdings_value
-      return BigDecimal("0") unless simplefin_account.raw_payload&.dig("holdings")
+    def holdings_data
+      @holdings_data ||= simplefin_account.raw_holdings_payload.presence ||
+                         simplefin_account.raw_payload&.dig("holdings") ||
+                         []
+    end
 
-      holdings_data = simplefin_account.raw_payload["holdings"]
+    def non_cash_holdings_value
+      return BigDecimal("0") unless holdings_data.present?
 
       holdings_data.sum do |holding|
-        market_value = holding["market_value"]
-        begin
-          case market_value
-          when String
-            BigDecimal(market_value)
-          when Numeric
-            BigDecimal(market_value.to_s)
-          else
-            BigDecimal("0")
-          end
-        rescue ArgumentError => e
-          Rails.logger.warn "SimpleFin holdings market_value parse error for account #{simplefin_account.account_id || simplefin_account.id}: #{e.message} (value: #{market_value.inspect})"
-          BigDecimal("0")
-        end
+        # Skip money market funds - they're cash equivalents
+        next BigDecimal("0") if cash_equivalent?(holding)
+
+        parse_market_value(holding["market_value"])
       end
     end
+
+    def cash_equivalent?(holding)
+      symbol = holding["symbol"].to_s.upcase.strip
+      description = holding["description"].to_s
+
+      # Check known money market tickers
+      return true if MONEY_MARKET_TICKERS.include?(symbol)
+
+      # Check description patterns
+      MONEY_MARKET_PATTERNS.any? { |pattern| description.match?(pattern) }
+    end
+
+    def parse_market_value(market_value)
+      case market_value
+      when String
+        BigDecimal(market_value)
+      when Numeric
+        BigDecimal(market_value.to_s)
+      else
+        BigDecimal("0")
+      end
+    rescue ArgumentError => e
+      Rails.logger.warn "SimpleFin holdings market_value parse error for account #{simplefin_account.account_id || simplefin_account.id}: #{e.message} (value: #{market_value.inspect})"
+      BigDecimal("0")
+    end
 end

app/models/simplefin_account/investments/holdings_processor.rb

@@ -63,8 +63,10 @@ class SimplefinAccount::Investments::HoldingsProcessor
           0
         end
 
-        # Use best-known date: created -> updated_at -> as_of -> date -> today
-        holding_date = parse_holding_date(any_of(simplefin_holding, %w[created updated_at as_of date])) || Date.current
+        # SimpleFIN holdings represent a current snapshot, not historical positions.
+        # Always use today's date regardless of the `created` timestamp (which is when
+        # the holding was first seen by SimpleFIN, not when we observed it).
+        holding_date = Date.current
 
         # Skip zero positions with no value to avoid invisible rows
         next if qty.to_d.zero? && computed_amount.to_d.zero?