diff --git a/app/controllers/simplefin_items_controller.rb b/app/controllers/simplefin_items_controller.rb index bc80ec0d7..271740cbc 100644 --- a/app/controllers/simplefin_items_controller.rb +++ b/app/controllers/simplefin_items_controller.rb @@ -1,6 +1,6 @@ class SimplefinItemsController < ApplicationController include SimplefinItems::MapsHelper - before_action :set_simplefin_item, only: [ :show, :edit, :update, :destroy, :sync, :balances, :setup_accounts, :complete_account_setup, :errors ] + before_action :set_simplefin_item, only: [ :show, :edit, :update, :destroy, :sync, :balances, :setup_accounts, :complete_account_setup ] def index @simplefin_items = Current.family.simplefin_items.active.ordered @@ -366,35 +366,6 @@ class SimplefinItemsController < ApplicationController end end - def errors - # Find the latest sync to surface its error messages in a lightweight modal - latest_sync = if @simplefin_item.syncs.loaded? - @simplefin_item.syncs.max_by(&:created_at) - else - @simplefin_item.syncs.ordered.first - end - - stats = (latest_sync&.sync_stats || {}) - raw_errors = Array(stats["errors"]) # may contain strings or hashes with message keys - - @errors = raw_errors.map { |e| - if e.is_a?(Hash) - e["message"] || e[:message] || e.to_s - else - e.to_s - end - }.compact - - # Fall back to simplefin_item.sync_error if present and not already included - if @simplefin_item.respond_to?(:sync_error) && @simplefin_item.sync_error.present? - @errors << @simplefin_item.sync_error - end - - # De-duplicate and keep non-empty messages - @errors = @errors.map(&:to_s).map(&:strip).reject(&:blank?).uniq - - render layout: false - end private diff --git a/app/helpers/simplefin_items_helper.rb b/app/helpers/simplefin_items_helper.rb index 9b3c631f1..d141bdf17 100644 --- a/app/helpers/simplefin_items_helper.rb +++ b/app/helpers/simplefin_items_helper.rb @@ -18,11 +18,15 @@ module SimplefinItemsHelper total_errors = stats["total_errors"].to_i return nil if total_errors.zero? - sample = Array(stats["errors"]).map do |e| + # Build a small, de-duplicated sample of messages with counts + grouped = Array(stats["errors"]).map { |e| name = (e[:name] || e["name"]).to_s msg = (e[:message] || e["message"]).to_s - name.present? ? "#{name}: #{msg}" : msg - end.compact.first(2).join(" • ") + text = name.present? ? "#{name}: #{msg}" : msg + text.strip + }.reject(&:blank?).tally + + sample = grouped.first(2).map { |text, count| count > 1 ? "#{text} (×#{count})" : text }.join(" • ") buckets = stats["error_buckets"] || {} bucket_text = if buckets.present? diff --git a/app/models/simplefin_item/importer.rb b/app/models/simplefin_item/importer.rb index 7745ea1d7..19038da8a 100644 --- a/app/models/simplefin_item/importer.rb +++ b/app/models/simplefin_item/importer.rb @@ -1,3 +1,4 @@ +require "set" class SimplefinItem::Importer class RateLimitedError < StandardError; end attr_reader :simplefin_item, :simplefin_provider, :sync @@ -54,13 +55,8 @@ class SimplefinItem::Importer import_account_minimal_and_balance(account_data) rescue => e stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1 - stats["errors"] ||= [] - stats["total_errors"] = stats.fetch("total_errors", 0) + 1 cat = classify_error(e) - buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 } - buckets[cat] = buckets.fetch(cat, 0) + 1 - stats["errors"] << { account_id: account_data[:id], name: account_data[:name], message: e.message.to_s, category: cat } - stats["errors"] = stats["errors"].last(5) + register_error(message: e.message, category: cat, account_id: account_data[:id], name: account_data[:name]) ensure persist_stats! end @@ -90,15 +86,11 @@ class SimplefinItem::Importer rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e # Surface a friendly duplicate/validation signal in sync stats and continue stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1 - stats["errors"] ||= [] - stats["total_errors"] = stats.fetch("total_errors", 0) + 1 - cat = "other" msg = e.message.to_s if msg.downcase.include?("already been taken") || msg.downcase.include?("unique") msg = "Duplicate upstream account detected for SimpleFin (account_id=#{account_id}). Try relinking to an existing manual account." end - stats["errors"] << { account_id: account_id, name: account_data[:name], message: msg, category: cat } - stats["errors"] = stats["errors"].last(5) + register_error(message: msg, category: "other", account_id: account_id, name: account_data[:name]) persist_stats! return end @@ -117,6 +109,49 @@ class SimplefinItem::Importer @stats ||= {} end + # Track seen error fingerprints during a single importer run to avoid double counting + def seen_errors + @seen_errors ||= Set.new + end + + # Register an error into stats with de-duplication and bucketing + def register_error(message:, category:, account_id: nil, name: nil) + msg = message.to_s.strip + cat = (category.presence || "other").to_s + fp = [ account_id.to_s.presence, cat, msg ].compact.join("|") + first_time = !seen_errors.include?(fp) + seen_errors.add(fp) + + if first_time + Rails.logger.warn( + "SimpleFin sync error (unique this run): category=#{cat} account_id=#{account_id.inspect} name=#{name.inspect} msg=#{msg}" + ) + # Emit an instrumentation event for observability dashboards + ActiveSupport::Notifications.instrument( + "simplefin.error", + item_id: simplefin_item.id, + account_id: account_id, + account_name: name, + category: cat, + message: msg + ) + else + # Keep logs tame; don't spam on repeats in the same run + end + + stats["errors"] ||= [] + buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 } + if first_time + stats["total_errors"] = stats.fetch("total_errors", 0) + 1 + buckets[cat] = buckets.fetch(cat, 0) + 1 + end + + # Maintain a small rolling sample (not de-duped so users can see most recent context) + stats["errors"] << { account_id: account_id, name: name, message: msg, category: cat } + stats["errors"] = stats["errors"].last(5) + persist_stats! + end + def persist_stats! return unless sync && sync.respond_to?(:sync_stats) merged = (sync.sync_stats || {}).merge(stats) @@ -196,21 +231,9 @@ class SimplefinItem::Importer rescue => e stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1 # Collect lightweight error info for UI stats - stats["errors"] ||= [] - stats["total_errors"] = stats.fetch("total_errors", 0) + 1 cat = classify_error(e) - buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 } - buckets[cat] = buckets.fetch(cat, 0) + 1 begin - err_item = { - account_id: account_data[:id], - name: account_data[:name], - message: e.message.to_s, - category: cat - } - stats["errors"] << err_item - # Keep only a small sample for UI (avoid blowing up sync_stats) - stats["errors"] = stats["errors"].last(5) + register_error(message: e.message.to_s, category: cat, account_id: account_data[:id], name: account_data[:name]) rescue # no-op if account_data is missing keys end @@ -255,19 +278,9 @@ class SimplefinItem::Importer import_account(account_data) rescue => e stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1 - stats["errors"] ||= [] - stats["total_errors"] = stats.fetch("total_errors", 0) + 1 cat = classify_error(e) - buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 } - buckets[cat] = buckets.fetch(cat, 0) + 1 begin - stats["errors"] << { - account_id: account_data[:id], - name: account_data[:name], - message: e.message.to_s, - category: cat - } - stats["errors"] = stats["errors"].last(5) + register_error(message: e.message.to_s, category: cat, account_id: account_data[:id], name: account_data[:name]) rescue # no-op if account_data is missing keys end @@ -308,19 +321,9 @@ class SimplefinItem::Importer import_account(account_data) rescue => e stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1 - stats["errors"] ||= [] - stats["total_errors"] = stats.fetch("total_errors", 0) + 1 cat = classify_error(e) - buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 } - buckets[cat] = buckets.fetch(cat, 0) + 1 begin - stats["errors"] << { - account_id: account_data[:id], - name: account_data[:name], - message: e.message.to_s, - category: cat - } - stats["errors"] = stats["errors"].last(5) + register_error(message: e.message.to_s, category: cat, account_id: account_data[:id], name: account_data[:name]) rescue # no-op if account_data is missing keys end @@ -464,15 +467,11 @@ class SimplefinItem::Importer rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e # Treat duplicates/validation failures as partial success: count and surface friendly error, then continue stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1 - stats["errors"] ||= [] - stats["total_errors"] = stats.fetch("total_errors", 0) + 1 - cat = "other" msg = e.message.to_s if msg.downcase.include?("already been taken") || msg.downcase.include?("unique") msg = "Duplicate upstream account detected for SimpleFin (account_id=#{account_id}). Try relinking to an existing manual account." end - stats["errors"] << { account_id: account_id, name: account_data[:name], message: msg, category: cat } - stats["errors"] = stats["errors"].last(5) + register_error(message: msg, category: "other", account_id: account_id, name: account_data[:name]) persist_stats! nil end @@ -498,11 +497,23 @@ class SimplefinItem::Importer end end - simplefin_item.update!(status: :requires_update) if needs_update + if needs_update + Rails.logger.warn("SimpleFin: marking item ##{simplefin_item.id} requires_update due to auth-related provider errors") + simplefin_item.update!(status: :requires_update) + ActiveSupport::Notifications.instrument( + "simplefin.item_requires_update", + item_id: simplefin_item.id, + reason: "provider_errors_partial", + count: arr.size + ) + end - stats["errors"] ||= [] - stats["total_errors"] = stats.fetch("total_errors", 0) + arr.size - buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 } + Rails.logger.info("SimpleFin: recording #{arr.size} non-fatal provider error(s) with partial data present") + ActiveSupport::Notifications.instrument( + "simplefin.provider_errors", + item_id: simplefin_item.id, + count: arr.size + ) arr.each do |error| msg = if error.is_a?(String) @@ -520,14 +531,8 @@ class SimplefinItem::Importer else "other" end - buckets[category] = buckets.fetch(category, 0) + 1 - stats["errors"] << { message: msg, category: category } + register_error(message: msg, category: category) end - - # Keep error sample small for UI - stats["errors"] = stats["errors"].last(5) - - persist_stats! end def handle_errors(errors) @@ -544,17 +549,31 @@ class SimplefinItem::Importer end if needs_update + Rails.logger.warn("SimpleFin: marking item ##{simplefin_item.id} requires_update due to fatal auth error(s): #{error_messages}") simplefin_item.update!(status: :requires_update) end + down = error_messages.downcase # Detect and surface rate-limit specifically with a friendlier exception - if error_messages.downcase.include?("make fewer requests") || - error_messages.downcase.include?("only refreshed once every 24 hours") || - error_messages.downcase.include?("rate limit") + if down.include?("make fewer requests") || + down.include?("only refreshed once every 24 hours") || + down.include?("rate limit") + Rails.logger.info("SimpleFin: raising RateLimitedError for item ##{simplefin_item.id}: #{error_messages}") + ActiveSupport::Notifications.instrument( + "simplefin.rate_limited", + item_id: simplefin_item.id, + message: error_messages + ) raise RateLimitedError, "SimpleFin rate limit: data refreshes at most once every 24 hours. Try again later." end # Fall back to generic SimpleFin error classified as :api_error + Rails.logger.error("SimpleFin fatal API error for item ##{simplefin_item.id}: #{error_messages}") + ActiveSupport::Notifications.instrument( + "simplefin.fatal_error", + item_id: simplefin_item.id, + message: error_messages + ) raise Provider::Simplefin::SimplefinError.new( "SimpleFin API errors: #{error_messages}", :api_error diff --git a/app/views/simplefin_items/_simplefin_item.html.erb b/app/views/simplefin_items/_simplefin_item.html.erb index b466d04d6..0a20f8950 100644 --- a/app/views/simplefin_items/_simplefin_item.html.erb +++ b/app/views/simplefin_items/_simplefin_item.html.erb @@ -57,7 +57,6 @@ <% if tooltip_text.present? %> <%= render DS::Tooltip.new(text: tooltip_text, icon: "alert-octagon", size: "sm", color: "warning") %> <% end %> - <%= render DS::Link.new(text: "View errors", icon: "alert-octagon", variant: "secondary", href: errors_simplefin_item_path(simplefin_item), frame: "modal") %> <% end %> <% if stats["total_accounts"].to_i > 0 %> diff --git a/app/views/simplefin_items/errors.html.erb b/app/views/simplefin_items/errors.html.erb deleted file mode 100644 index 6a93d1634..000000000 --- a/app/views/simplefin_items/errors.html.erb +++ /dev/null @@ -1,29 +0,0 @@ -<%# Modal: Show SimpleFIN sync errors for a connection %> -<%= turbo_frame_tag "modal" do %> - <%= render DS::Dialog.new do |dialog| %> - <% dialog.with_header(title: "SimpleFIN sync errors") %> - - <% dialog.with_body do %> - <% if @errors.present? %> -
-

We found the following errors in the latest sync:

- -
- <% else %> -
-

No errors were recorded for the latest sync.

-
- <% end %> - <% end %> - - <% dialog.with_footer do %> -
- <%= render DS::Link.new(text: "Close", variant: :secondary, href: accounts_path, data: { turbo_frame: "_top" }) %> -
- <% end %> - <% end %> -<% end %> \ No newline at end of file diff --git a/config/routes.rb b/config/routes.rb index af25a0a40..b07d53b3b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -303,7 +303,6 @@ Rails.application.routes.draw do member do post :sync post :balances - get :errors get :setup_accounts post :complete_account_setup end