Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions app/models/account/balance_sync_window.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# frozen_string_literal: true

# Resolves the earliest date from which an account balance sync should recalculate
# when no explicit window was passed on the parent sync.
class Account::BalanceSyncWindow
LOOKBACK = 7.days

class << self
# @param account [Account]
# @param parent_sync [Sync, nil] Used to detect entries created/updated during this sync run
# @param parent_window_start_date [Date, nil] Explicit window from caller or parent sync
# @param import_window_start_date [Date, nil] Transaction fetch window from provider import
# @param last_synced_at [Time, nil] Provider item freshness timestamp
# @return [Date, nil] nil means full balance recalculation
def for_account(account, parent_sync: nil, parent_window_start_date: nil, import_window_start_date: nil, last_synced_at: nil)
return parent_window_start_date.to_date if parent_window_start_date.present?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Apply opening-anchor floor even for explicit parent windows.

Returning early here bypasses the floor in Lines 26-27, so an explicit parent window can go earlier than the account’s valid floor. That breaks the incremental-window contract and can trigger unnecessary backfill recalculation.

Proposed fix
-      return parent_window_start_date.to_date if parent_window_start_date.present?
+      candidates = []
+      candidates << parent_window_start_date.to_date if parent_window_start_date.present?
 
-      candidates = []
       candidates << import_window_start_date.to_date if import_window_start_date.present?
       candidates << entries_touched_since(parent_sync, account) if parent_sync
       candidates << (last_synced_at.to_date - LOOKBACK) if last_synced_at.present?
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return parent_window_start_date.to_date if parent_window_start_date.present?
candidates = []
candidates << parent_window_start_date.to_date if parent_window_start_date.present?
candidates << import_window_start_date.to_date if import_window_start_date.present?
candidates << entries_touched_since(parent_sync, account) if parent_sync
candidates << (last_synced_at.to_date - LOOKBACK) if last_synced_at.present?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/account/balance_sync_window.rb` at line 16, The current early
return of parent_window_start_date.to_date bypasses applying the account-level
opening-anchor floor and allows parent windows earlier than allowed; change the
logic so that when parent_window_start_date is present you convert it to a date,
then compare it with opening_anchor_floor and return the later date (e.g.,
compute start = parent_window_start_date.to_date and return [start,
opening_anchor_floor].max) so the opening-anchor floor is always enforced;
modify the method containing parent_window_start_date and opening_anchor_floor
references accordingly.


candidates = []
candidates << import_window_start_date.to_date if import_window_start_date.present?
candidates << entries_touched_since(parent_sync, account) if parent_sync
candidates << (last_synced_at.to_date - LOOKBACK) if last_synced_at.present?

window = candidates.compact.min
return nil unless window

floor = [ account.opening_anchor_date, account.start_date ].compact.max
[ window, floor ].max
end

private

def entries_touched_since(sync, account)
sync_started_at = sync.created_at
return nil unless sync_started_at

account.entries
.where("entries.created_at >= :t OR entries.updated_at >= :t", t: sync_started_at)
.minimum(:date)
end
end
end
93 changes: 93 additions & 0 deletions app/models/account/schedules_balance_syncs.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# frozen_string_literal: true

# Shared logic for provider items scheduling per-account balance syncs after import.
module Account::SchedulesBalanceSyncs
extend ActiveSupport::Concern

# @param accounts [Enumerable<Account>, nil] Defaults to {#balance_sync_accounts}
# @return [Array<Hash>, nil] Result rows when +report_results+ is true; otherwise nil
def schedule_account_syncs(parent_sync: nil, window_start_date: nil, window_end_date: nil, import_window_start_date: nil, accounts: nil, report_results: schedule_account_syncs_report_results?)
schedule_account_syncs_for(
accounts || balance_sync_accounts,
parent_sync: parent_sync,
window_start_date: window_start_date,
window_end_date: window_end_date,
import_window_start_date: import_window_start_date,
report_results: report_results
)
end

def schedule_account_syncs_for(accounts, parent_sync: nil, window_start_date: nil, window_end_date: nil, import_window_start_date: nil, report_results: schedule_account_syncs_report_results?)
return [] if report_results && accounts.blank?

end_date = window_end_date || parent_sync&.window_end_date
last_synced = balance_sync_last_synced_at

iterator = report_results ? :schedule_with_results : :schedule_each
send(iterator, accounts, parent_sync:, window_start_date:, end_date:, import_window_start_date:, last_synced:)
end

private

def schedule_account_syncs_report_results?
false
end

def balance_sync_accounts
accts = accounts
accts.respond_to?(:visible) ? accts.visible : accts
end

def balance_sync_last_synced_at
last_synced_at if respond_to?(:last_synced_at)
end

def schedule_each(accounts, parent_sync:, window_start_date:, end_date:, import_window_start_date:, last_synced:)
accounts.each do |account|
schedule_balance_sync_for_account(
account,
parent_sync: parent_sync,
window_start_date: window_start_date,
window_end_date: end_date,
import_window_start_date: import_window_start_date,
last_synced_at: last_synced
)
end
nil
end

def schedule_with_results(accounts, parent_sync:, window_start_date:, end_date:, import_window_start_date:, last_synced:)
results = []
accounts.each do |account|
schedule_balance_sync_for_account(
account,
parent_sync: parent_sync,
window_start_date: window_start_date,
window_end_date: end_date,
import_window_start_date: import_window_start_date,
last_synced_at: last_synced
)
results << { account_id: account.id, success: true }
rescue => e
Rails.logger.error "#{self.class.name} #{id} - Failed to schedule sync for account #{account.id}: #{e.message}"
results << { account_id: account.id, success: false, error: e.message }
end
results
end

def schedule_balance_sync_for_account(account, parent_sync:, window_start_date:, window_end_date:, import_window_start_date:, last_synced_at:)
effective_window = Account::BalanceSyncWindow.for_account(
account,
parent_sync: parent_sync,
parent_window_start_date: window_start_date || parent_sync&.window_start_date,
import_window_start_date: import_window_start_date,
last_synced_at: last_synced_at
)

account.sync_later(
parent_sync: parent_sync,
window_start_date: effective_window,
window_end_date: window_end_date
)
end
end
7 changes: 4 additions & 3 deletions app/models/balance/materializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,15 @@ def persist_balances
)
end

# Called only from #materialize_balances after #calculate_balances, so calculator.incremental? is safe.
def purge_stale_balances
sorted_balances = @balances.sort_by(&:date)

if sorted_balances.empty?
# In incremental forward-sync, even when no balances were calculated for the window
# (e.g. window_start_date is beyond the last entry), purge stale tail records that
# now fall beyond the prior-balance boundary so orphaned future rows are cleaned up.
if strategy == :forward && calculator.incremental? && account.opening_anchor_date <= @window_start_date - 1
if @window_start_date.present? && calculator.respond_to?(:incremental?) && calculator.incremental? && account.opening_anchor_date <= @window_start_date - 1
deleted_count = account.balances.delete_by(
"date < ? OR date > ?",
account.opening_anchor_date,
Expand All @@ -98,7 +99,7 @@ def purge_stale_balances
# Use opening_anchor_date as the lower purge bound to preserve them.
# We ask the calculator whether it actually ran incrementally — it may have
# fallen back to a full recalculation, in which case we use the normal bound.
oldest_valid_date = if strategy == :forward && calculator.incremental?
oldest_valid_date = if calculator.respond_to?(:incremental?) && calculator.incremental?
account.opening_anchor_date
else
sorted_balances.first.date
Expand All @@ -110,7 +111,7 @@ def purge_stale_balances

def calculator
@calculator ||= if strategy == :reverse
Balance::ReverseCalculator.new(account)
Balance::ReverseCalculator.new(account, window_start_date: @window_start_date)
else
Balance::ForwardCalculator.new(account, window_start_date: @window_start_date)
end
Expand Down
50 changes: 49 additions & 1 deletion app/models/balance/reverse_calculator.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,16 @@
class Balance::ReverseCalculator < Balance::BaseCalculator
def initialize(account, window_start_date: nil)
super(account)
@window_start_date = window_start_date
@fell_back = nil
end

# True when a window was provided and we successfully limited recalculation to that range.
def incremental?
raise "incremental? must not be called before calculate" if @window_start_date.present? && @fell_back.nil?
@window_start_date.present? && @fell_back == false
end

def calculate
Rails.logger.tagged("Balance::ReverseCalculator") do
# Since it's a reverse sync, we're starting with the "end of day" balance components and
Expand All @@ -9,8 +21,10 @@ def calculate
)
end_non_cash_balance = account.current_anchor_balance - end_cash_balance

calc_start_date = resolve_calc_start_date

# Calculates in reverse-chronological order (End of day -> Start of day)
account.current_anchor_date.downto(account.opening_anchor_date).map do |date|
account.current_anchor_date.downto(calc_start_date).map do |date|
flows = flows_for_date(date)
valuation = sync_cache.get_valuation(date)

Expand Down Expand Up @@ -68,6 +82,40 @@ def calculate

private

def resolve_calc_start_date
if @window_start_date.present?
if multi_currency_account?
Rails.logger.info("Account has multi-currency entries or is foreign, falling back to full reverse recalculation")
@fell_back = true
return account.opening_anchor_date
end

prior = prior_balance

if prior
Rails.logger.info("Incremental reverse sync from #{@window_start_date}, preserving balances before #{@window_start_date}")
@fell_back = false
return [ @window_start_date, account.opening_anchor_date ].max
Comment on lines +95 to +98
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fall back for reverse sync unless boundary balance is enforced

When window_start_date is present, this branch enables incremental reverse calculation solely based on the existence of prior_balance, but the reverse algorithm never seeds from that prior balance or validates the boundary. In reverse mode, days before window_start_date depend on the recomputed balance at window_start_date, so any changed flow/valuation on or after the window can require updates to window_start_date - 1 and earlier. Because the materializer now preserves pre-window rows in incremental mode, linked accounts can keep stale historical balances after provider imports.

Useful? React with 👍 / 👎.

else
Rails.logger.info("No persisted balance found for #{@window_start_date - 1}, falling back to full reverse recalculation")
@fell_back = true
end
end

account.opening_anchor_date
end

def multi_currency_account?
account.entries.where.not(currency: account.currency).exists? ||
account.currency != account.family.currency
end

def prior_balance
account.balances
.where(currency: account.currency)
.find_by(date: @window_start_date - 1)
end

# Negative entries amount on an "asset" account means, "account value has increased"
# Negative entries amount on a "liability" account means, "account debt has decreased"
# Positive entries amount on an "asset" account means, "account value has decreased"
Expand Down
29 changes: 7 additions & 22 deletions app/models/binance_item.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

class BinanceItem < ApplicationRecord
include Syncable, Provided, Unlinking, Encryptable
include Syncable, Provided, Unlinking, Encryptable, Account::SchedulesBalanceSyncs

enum :status, { good: "good", requires_update: "requires_update" }, default: :good

Expand Down Expand Up @@ -78,27 +78,6 @@ def process_accounts
results
end

def schedule_account_syncs(parent_sync: nil, window_start_date: nil, window_end_date: nil)
return [] if accounts.empty?

results = []
accounts.visible.each do |account|
begin
account.sync_later(
parent_sync: parent_sync,
window_start_date: window_start_date,
window_end_date: window_end_date
)
results << { account_id: account.id, success: true }
rescue StandardError => e
Rails.logger.error "BinanceItem #{id} - Failed to schedule sync for account #{account.id}: #{e.message}"
results << { account_id: account.id, success: false, error: e.message }
end
end

results
end

def upsert_binance_snapshot!(payload)
update!(raw_payload: payload)
end
Expand Down Expand Up @@ -156,4 +135,10 @@ def set_binance_institution_defaults!
institution_color: "#F0B90B"
)
end

private

def schedule_account_syncs_report_results?
true
end
end
28 changes: 6 additions & 22 deletions app/models/brex_item.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class BrexItem < ApplicationRecord
include Syncable, Provided, Unlinking, Encryptable
include Syncable, Provided, Unlinking, Encryptable, Account::SchedulesBalanceSyncs

BLANK_TOKEN_SENTINELS = [ "", " ", " ", " ", "\t", "\n", "\r" ].freeze

Expand Down Expand Up @@ -75,27 +75,6 @@ def process_accounts
results
end

def schedule_account_syncs(parent_sync: nil, window_start_date: nil, window_end_date: nil)
return [] if accounts.empty?

results = []
accounts.visible.each do |account|
begin
account.sync_later(
parent_sync: parent_sync,
window_start_date: window_start_date,
window_end_date: window_end_date
)
results << { account_id: account.id, success: true }
rescue => e
Rails.logger.error "BrexItem #{id} - Failed to schedule sync for account #{account.id}: #{e.message}"
results << { account_id: account.id, success: false, error: e.message }
end
end

results
end

def upsert_brex_snapshot!(accounts_snapshot)
update!(raw_payload: BrexAccount.sanitize_payload(accounts_snapshot))
end
Expand Down Expand Up @@ -168,6 +147,11 @@ def effective_base_url
end

private

def schedule_account_syncs_report_results?
true
end

def normalize_token
self.token = token&.strip
end
Expand Down
29 changes: 7 additions & 22 deletions app/models/coinbase_item.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class CoinbaseItem < ApplicationRecord
include Syncable, Provided, Unlinking
include Syncable, Provided, Unlinking, Account::SchedulesBalanceSyncs
include Encryptable

enum :status, { good: "good", requires_update: "requires_update" }, default: :good
Expand Down Expand Up @@ -78,27 +78,6 @@ def process_accounts
results
end

def schedule_account_syncs(parent_sync: nil, window_start_date: nil, window_end_date: nil)
return [] if accounts.empty?

results = []
accounts.visible.each do |account|
begin
account.sync_later(
parent_sync: parent_sync,
window_start_date: window_start_date,
window_end_date: window_end_date
)
results << { account_id: account.id, success: true }
rescue => e
Rails.logger.error "CoinbaseItem #{id} - Failed to schedule sync for account #{account.id}: #{e.message}"
results << { account_id: account.id, success: false, error: e.message }
end
end

results
end

def upsert_coinbase_snapshot!(accounts_snapshot)
assign_attributes(
raw_payload: accounts_snapshot
Expand Down Expand Up @@ -174,4 +153,10 @@ def set_coinbase_institution_defaults!
institution_color: "#0052FF"
)
end

private

def schedule_account_syncs_report_results?
true
end
end
Loading
Loading