From ab2cec55e77d9e5b1ebb17992ada6dc117b7eefd Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 9 May 2025 14:56:49 -0400 Subject: [PATCH 1/9] Propagate child sync errors up to parent, fix sync status (#2232) * Propagate child sync errors up to parent, fix sync status * Remove testing error --- app/controllers/concerns/auto_sync.rb | 1 - app/models/account.rb | 2 -- app/models/concerns/syncable.rb | 6 ++++- app/models/family.rb | 2 -- app/models/plaid_item.rb | 2 -- app/models/sync.rb | 23 +++++++++++++------ .../20250509182903_dynamic_last_synced.rb | 7 ++++++ db/schema.rb | 5 +--- test/fixtures/families.yml | 3 --- 9 files changed, 29 insertions(+), 22 deletions(-) create mode 100644 db/migrate/20250509182903_dynamic_last_synced.rb diff --git a/app/controllers/concerns/auto_sync.rb b/app/controllers/concerns/auto_sync.rb index 970eec0a..4e375359 100644 --- a/app/controllers/concerns/auto_sync.rb +++ b/app/controllers/concerns/auto_sync.rb @@ -7,7 +7,6 @@ module AutoSync private def sync_family - Current.family.update!(last_synced_at: Time.current) Current.family.sync_later end diff --git a/app/models/account.rb b/app/models/account.rb index 85167e22..352335e0 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -82,8 +82,6 @@ class Account < ApplicationRecord end def sync_data(sync, start_date: nil) - update!(last_synced_at: Time.current) - Rails.logger.info("Processing balances (#{linked? ? 'reverse' : 'forward'})") sync_balances end diff --git a/app/models/concerns/syncable.rb b/app/models/concerns/syncable.rb index 0ae45122..d804b992 100644 --- a/app/models/concerns/syncable.rb +++ b/app/models/concerns/syncable.rb @@ -27,7 +27,11 @@ module Syncable end def sync_error - latest_sync.error + latest_sync&.error + end + + def last_synced_at + latest_sync&.last_ran_at end private diff --git a/app/models/family.rb b/app/models/family.rb index 3e877dc1..f565d54d 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -66,8 +66,6 @@ class Family < ApplicationRecord end def sync_data(sync, start_date: nil) - update!(last_synced_at: Time.current) - # We don't rely on this value to guard the app, but keep it eventually consistent sync_trial_status! diff --git a/app/models/plaid_item.rb b/app/models/plaid_item.rb index a527dc3e..2226f12f 100644 --- a/app/models/plaid_item.rb +++ b/app/models/plaid_item.rb @@ -38,8 +38,6 @@ class PlaidItem < ApplicationRecord end def sync_data(sync, start_date: nil) - update!(last_synced_at: Time.current) - begin Rails.logger.info("Fetching and loading Plaid data") fetch_and_load_plaid_data(sync) diff --git a/app/models/sync.rb b/app/models/sync.rb index 22b61829..826899ce 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -28,8 +28,7 @@ class Sync < ApplicationRecord Rails.logger.info("Post-sync completed") end rescue StandardError => error - fail! error - raise error if Rails.env.development? + fail! error, report_error: true ensure notify_parent_of_completion! if has_parent? end @@ -43,7 +42,11 @@ class Sync < ApplicationRecord self.lock! unless has_pending_child_syncs? - complete! + if has_failed_child_syncs? + fail!(Error.new("One or more child syncs failed")) + else + complete! + end # If this sync is both a child and a parent, we need to notify the parent of completion notify_parent_of_completion! if has_parent? @@ -58,6 +61,10 @@ class Sync < ApplicationRecord children.where(status: [ :pending, :syncing ]).any? end + def has_failed_child_syncs? + children.where(status: :failed).any? + end + def has_parent? parent_id.present? end @@ -76,12 +83,14 @@ class Sync < ApplicationRecord update! status: :completed, last_ran_at: Time.current end - def fail!(error) + def fail!(error, report_error: false) Rails.logger.error("Sync failed: #{error.message}") - Sentry.capture_exception(error) do |scope| - scope.set_context("sync", { id: id, syncable_type: syncable_type, syncable_id: syncable_id }) - scope.set_tags(sync_id: id) + if report_error + Sentry.capture_exception(error) do |scope| + scope.set_context("sync", { id: id, syncable_type: syncable_type, syncable_id: syncable_id }) + scope.set_tags(sync_id: id) + end end update!( diff --git a/db/migrate/20250509182903_dynamic_last_synced.rb b/db/migrate/20250509182903_dynamic_last_synced.rb new file mode 100644 index 00000000..6ade36eb --- /dev/null +++ b/db/migrate/20250509182903_dynamic_last_synced.rb @@ -0,0 +1,7 @@ +class DynamicLastSynced < ActiveRecord::Migration[7.2] + def change + remove_column :plaid_items, :last_synced_at, :datetime + remove_column :accounts, :last_synced_at, :datetime + remove_column :families, :last_synced_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 9a7e5fcf..6d76a125 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_05_02_164951) do +ActiveRecord::Schema[7.2].define(version: 2025_05_09_182903) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -34,7 +34,6 @@ ActiveRecord::Schema[7.2].define(version: 2025_05_02_164951) do t.uuid "import_id" t.uuid "plaid_account_id" t.boolean "scheduled_for_deletion", default: false - t.datetime "last_synced_at" t.decimal "cash_balance", precision: 19, scale: 4, default: "0.0" t.jsonb "locked_attributes", default: {} t.index ["accountable_id", "accountable_type"], name: "index_accounts_on_accountable_id_and_accountable_type" @@ -225,7 +224,6 @@ ActiveRecord::Schema[7.2].define(version: 2025_05_02_164951) do t.string "stripe_customer_id" t.string "date_format", default: "%m-%d-%Y" t.string "country", default: "US" - t.datetime "last_synced_at" t.string "timezone" t.boolean "data_enrichment_enabled", default: false t.boolean "early_access", default: false @@ -445,7 +443,6 @@ ActiveRecord::Schema[7.2].define(version: 2025_05_02_164951) do t.datetime "updated_at", null: false t.string "available_products", default: [], array: true t.string "billed_products", default: [], array: true - t.datetime "last_synced_at" t.string "plaid_region", default: "us", null: false t.string "institution_url" t.string "institution_id" diff --git a/test/fixtures/families.yml b/test/fixtures/families.yml index 9c6790b9..10d5bd18 100644 --- a/test/fixtures/families.yml +++ b/test/fixtures/families.yml @@ -1,8 +1,5 @@ empty: name: Family - last_synced_at: <%= Time.now %> dylan_family: name: The Dylan Family - last_synced_at: <%= Time.now %> - From 7605b0221d839e691d430682e3e3e6f97a67d2d5 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 9 May 2025 15:56:48 -0400 Subject: [PATCH 2/9] Batch upsert security prices on sync --- app/models/security/provided.rb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index b342c9e5..df81094b 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -63,7 +63,21 @@ module Security::Provided is_invalid end - Security::Price.upsert_all(valid_prices, unique_by: %i[security_id date currency]) + valid_prices.each_slice(100) do |batch| + retries ||= 0 + + begin + Security::Price.upsert_all(batch, unique_by: %i[security_id date currency]) + rescue => e + if retries < 3 + retries += 1 + sleep(1) + retry + else + raise e + end + end + end end def find_or_fetch_price(date: Date.current, cache: true) From f07c41821e70bb42f96cee249c9de289af05d850 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 9 May 2025 16:05:10 -0400 Subject: [PATCH 3/9] Add warn log for security price upsert retries --- app/models/security/provided.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index df81094b..d3f59b65 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -72,6 +72,7 @@ module Security::Provided if retries < 3 retries += 1 sleep(1) + Rails.logger.warn("Retrying upsert of #{batch.size} prices for security_id=#{id} ticker=#{ticker} retry=#{retries} error=#{e.message}") retry else raise e From 5d798fe0a0436f30ffce42e02cbcb6aaa0e5f4b3 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 9 May 2025 16:31:16 -0400 Subject: [PATCH 4/9] Remove retry logic from security upsert --- app/models/security/provided.rb | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index d3f59b65..351e3a3f 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -64,20 +64,7 @@ module Security::Provided end valid_prices.each_slice(100) do |batch| - retries ||= 0 - - begin - Security::Price.upsert_all(batch, unique_by: %i[security_id date currency]) - rescue => e - if retries < 3 - retries += 1 - sleep(1) - Rails.logger.warn("Retrying upsert of #{batch.size} prices for security_id=#{id} ticker=#{ticker} retry=#{retries} error=#{e.message}") - retry - else - raise e - end - end + Security::Price.upsert_all(batch, unique_by: %i[security_id date currency]) end end From 48a07d6158e5c905f306242d100bd55948414375 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 9 May 2025 16:42:44 -0400 Subject: [PATCH 5/9] Revert batch upserting --- app/models/security/provided.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index 351e3a3f..b342c9e5 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -63,9 +63,7 @@ module Security::Provided is_invalid end - valid_prices.each_slice(100) do |batch| - Security::Price.upsert_all(batch, unique_by: %i[security_id date currency]) - end + Security::Price.upsert_all(valid_prices, unique_by: %i[security_id date currency]) end def find_or_fetch_price(date: Date.current, cache: true) From 0006b6f6ca7cc1f0a56ee6d551f72419a104c941 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 9 May 2025 16:59:23 -0400 Subject: [PATCH 6/9] Add env to toggle provider price syncs --- app/models/security/provided.rb | 2 ++ test/test_helper.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index b342c9e5..e230b82f 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -29,6 +29,8 @@ module Security::Provided end def sync_provider_prices(start_date:, end_date: Date.current) + return 0 unless ENV["PROVIDER_PRICE_SYNC_ENABLED"] == "true" + unless has_prices? Rails.logger.warn("Security id=#{id} ticker=#{ticker} is not known by provider, skipping price sync") return 0 diff --git a/test/test_helper.rb b/test/test_helper.rb index 23f98faa..b586da49 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -13,6 +13,8 @@ ENV["RAILS_ENV"] ||= "test" # https://github.com/ged/ruby-pg/issues/538#issuecomment-1591629049 ENV["PGGSSENCMODE"] = "disable" +ENV["PROVIDER_PRICE_SYNC_ENABLED"] = "true" + require "rails/test_help" require "minitest/mock" require "minitest/autorun" From a268c5a563a641f69ebb62be627ebbb4f1fe069a Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 9 May 2025 17:47:35 -0400 Subject: [PATCH 7/9] Revert "Add env to toggle provider price syncs" This reverts commit 0006b6f6ca7cc1f0a56ee6d551f72419a104c941. --- app/models/security/provided.rb | 2 -- test/test_helper.rb | 2 -- 2 files changed, 4 deletions(-) diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index e230b82f..b342c9e5 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -29,8 +29,6 @@ module Security::Provided end def sync_provider_prices(start_date:, end_date: Date.current) - return 0 unless ENV["PROVIDER_PRICE_SYNC_ENABLED"] == "true" - unless has_prices? Rails.logger.warn("Security id=#{id} ticker=#{ticker} is not known by provider, skipping price sync") return 0 diff --git a/test/test_helper.rb b/test/test_helper.rb index b586da49..23f98faa 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -13,8 +13,6 @@ ENV["RAILS_ENV"] ||= "test" # https://github.com/ged/ruby-pg/issues/538#issuecomment-1591629049 ENV["PGGSSENCMODE"] = "disable" -ENV["PROVIDER_PRICE_SYNC_ENABLED"] = "true" - require "rails/test_help" require "minitest/mock" require "minitest/autorun" From 908b3e24899a69591de8db641522de9eac51b209 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 12 May 2025 15:41:14 -0400 Subject: [PATCH 8/9] Temporary disable of sync cascade behavior --- app/models/sync.rb | 49 ++++------------------------------------ test/models/sync_test.rb | 40 -------------------------------- 2 files changed, 4 insertions(+), 85 deletions(-) diff --git a/app/models/sync.rb b/app/models/sync.rb index 826899ce..4d923f12 100644 --- a/app/models/sync.rb +++ b/app/models/sync.rb @@ -21,58 +21,17 @@ class Sync < ApplicationRecord begin syncable.sync_data(self, start_date: start_date) - unless has_pending_child_syncs? - complete! - Rails.logger.info("Sync completed, starting post-sync") - syncable.post_sync(self) - Rails.logger.info("Post-sync completed") - end + complete! + Rails.logger.info("Sync completed, starting post-sync") + syncable.post_sync(self) + Rails.logger.info("Post-sync completed") rescue StandardError => error fail! error, report_error: true - ensure - notify_parent_of_completion! if has_parent? - end - end - end - - def handle_child_completion_event - Sync.transaction do - # We need this to ensure 2 child syncs don't update the parent at the exact same time with different results - # and cause the sync to hang in "syncing" status indefinitely - self.lock! - - unless has_pending_child_syncs? - if has_failed_child_syncs? - fail!(Error.new("One or more child syncs failed")) - else - complete! - end - - # If this sync is both a child and a parent, we need to notify the parent of completion - notify_parent_of_completion! if has_parent? - - syncable.post_sync(self) end end end private - def has_pending_child_syncs? - children.where(status: [ :pending, :syncing ]).any? - end - - def has_failed_child_syncs? - children.where(status: :failed).any? - end - - def has_parent? - parent_id.present? - end - - def notify_parent_of_completion! - parent.handle_child_completion_event - end - def start! Rails.logger.info("Starting sync") update! status: :syncing diff --git a/test/models/sync_test.rb b/test/models/sync_test.rb index 6b246a1f..99019146 100644 --- a/test/models/sync_test.rb +++ b/test/models/sync_test.rb @@ -31,44 +31,4 @@ class SyncTest < ActiveSupport::TestCase assert_equal "failed", @sync.status assert_equal "test sync error", @sync.error end - - # Order is important here. Parent syncs must implement sync_data so that their own work - # is 100% complete *prior* to queueing up child syncs. - test "runs sync with child syncs" do - family = families(:dylan_family) - - parent = Sync.create!(syncable: family) - child1 = Sync.create!(syncable: family.accounts.first, parent: parent) - child2 = Sync.create!(syncable: family.accounts.second, parent: parent) - grandchild = Sync.create!(syncable: family.accounts.last, parent: child2) - - parent.syncable.expects(:sync_data).returns([]).once - child1.syncable.expects(:sync_data).returns([]).once - child2.syncable.expects(:sync_data).returns([]).once - grandchild.syncable.expects(:sync_data).returns([]).once - - assert_equal "pending", parent.status - assert_equal "pending", child1.status - assert_equal "pending", child2.status - assert_equal "pending", grandchild.status - - parent.perform - assert_equal "syncing", parent.reload.status - - child1.perform - assert_equal "completed", child1.reload.status - assert_equal "syncing", parent.reload.status - - child2.perform - assert_equal "syncing", child2.reload.status - assert_equal "completed", child1.reload.status - assert_equal "syncing", parent.reload.status - - # Will complete the parent and grandparent syncs - grandchild.perform - assert_equal "completed", grandchild.reload.status - assert_equal "completed", child1.reload.status - assert_equal "completed", child2.reload.status - assert_equal "completed", parent.reload.status - end end From 9e6e4b1ce6937879d0061ca5ab845bf8b629652f Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 12 May 2025 18:55:19 -0400 Subject: [PATCH 9/9] Only run Plaid syncs via webhook after initial sync --- app/models/family.rb | 5 ----- app/views/accounts/index.html.erb | 19 ++++++++++--------- app/views/accounts/show/_header.html.erb | 17 +++-------------- app/views/plaid_items/_plaid_item.html.erb | 2 +- test/models/family_test.rb | 4 ---- 5 files changed, 14 insertions(+), 33 deletions(-) diff --git a/app/models/family.rb b/app/models/family.rb index f565d54d..a3a73eec 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -74,11 +74,6 @@ class Family < ApplicationRecord account.sync_later(start_date: start_date, parent_sync: sync) end - Rails.logger.info("Syncing plaid items for family #{id}") - plaid_items.each do |plaid_item| - plaid_item.sync_later(start_date: start_date, parent_sync: sync) - end - Rails.logger.info("Applying rules for family #{id}") rules.each do |rule| rule.apply_later diff --git a/app/views/accounts/index.html.erb b/app/views/accounts/index.html.erb index 4893e44c..b4c78332 100644 --- a/app/views/accounts/index.html.erb +++ b/app/views/accounts/index.html.erb @@ -2,15 +2,16 @@

<%= t(".accounts") %>

- <%= render ButtonComponent.new( - text: "Sync all", - href: sync_all_accounts_path, - method: :post, - variant: "outline", - disabled: Current.family.syncing?, - icon: "refresh-cw", - class: "" - ) %> + <% if Rails.env.development? %> + <%= render ButtonComponent.new( + text: "Sync all", + href: sync_all_accounts_path, + method: :post, + variant: "outline", + disabled: Current.family.syncing?, + icon: "refresh-cw", + ) %> + <% end %> <%= render LinkComponent.new( text: "New account", diff --git a/app/views/accounts/show/_header.html.erb b/app/views/accounts/show/_header.html.erb index 4c60e832..b64408ce 100644 --- a/app/views/accounts/show/_header.html.erb +++ b/app/views/accounts/show/_header.html.erb @@ -20,26 +20,15 @@ <% end %>
- <% if account.plaid_account_id.present? %> - <% if Rails.env.development? %> - <%= icon( + <% if Rails.env.development? %> + <%= icon( "refresh-cw", as_button: true, size: "sm", - href: sync_plaid_item_path(account.plaid_account.plaid_item), + href: account.linked? ? sync_plaid_item_path(account.plaid_account.plaid_item) : sync_account_path(account), disabled: account.syncing?, frame: :_top ) %> - <% end %> - <% else %> - <%= icon( - "refresh-cw", - as_button: true, - size: "sm", - href: sync_account_path(account), - disabled: account.syncing?, - frame: :_top - ) %> <% end %> <%= render "accounts/show/menu", account: account %> diff --git a/app/views/plaid_items/_plaid_item.html.erb b/app/views/plaid_items/_plaid_item.html.erb index 7c3dc8b2..61dea7dc 100644 --- a/app/views/plaid_items/_plaid_item.html.erb +++ b/app/views/plaid_items/_plaid_item.html.erb @@ -92,7 +92,7 @@
<% end %> - <% else %> + <% elsif Rails.env.development? %> <%= icon( "refresh-cw", as_button: true, diff --git a/test/models/family_test.rb b/test/models/family_test.rb index 7223e64b..24876a77 100644 --- a/test/models/family_test.rb +++ b/test/models/family_test.rb @@ -20,10 +20,6 @@ class FamilyTest < ActiveSupport::TestCase .with(start_date: nil, parent_sync: family_sync) .times(manual_accounts_count) - PlaidItem.any_instance.expects(:sync_later) - .with(start_date: nil, parent_sync: family_sync) - .times(items_count) - @syncable.sync_data(family_sync, start_date: family_sync.start_date) end end