Skip to content

perf(sync): bound family-level transfer matching to 90-day window#2444

Open
JSONbored wants to merge 1 commit into
we-promise:mainfrom
JSONbored:perf/sync-transfer-match-date-bound
Open

perf(sync): bound family-level transfer matching to 90-day window#2444
JSONbored wants to merge 1 commit into
we-promise:mainfrom
JSONbored:perf/sync-transfer-match-date-bound

Conversation

@JSONbored

@JSONbored JSONbored commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Closes #2447

Problem

Family::Syncer#perform_post_sync called auto_match_transfers! with no date filter, causing a full cartesian scan across all entries in a family's accounts on every sync.

For families with 10,000+ entries, this is an O(N²) self-join on the entries table. Sentry reports this as SURE-APP-WX: 18,894 occurrences, still firing as of today, only affecting 5 accounts (users with large transaction histories).

The offending query scans every Transaction entry for the family, finds every pair that could be a transfer (matching amount, opposite sign, within 4-day window), and sorts them — with no upper bound on how many entries it examines.

Fix

Add a since_date: parameter to transfer_match_candidates and auto_match_transfers!, and apply it as AND (:since_date IS NULL OR inflow_candidates.date >= :since_date) in both UNION halves of the SQL.

Family::Syncer#perform_post_sync now passes since_date: 90.days.ago.to_date, limiting each family-level post-sync scan to entries from the last 90 days.

Why 90 days is safe:

  • Ongoing syncs add recent transactions — nearly all new entries fall within the 90-day window
  • Historical transfers (>90 days) are already matched from prior syncs; the auto-match is idempotent and skips already-matched pairs
  • For new bank connections with historical imports, the account-level sync path fires per-account and keeps since_date: nil (full history for that specific account)
  • Manual entry scenarios also go through the account-level path

Callers unchanged:

  • account.family.auto_match_transfers!(account: account) — account-level path, no since_date → still full history for that account
  • Direct transfer_match_candidates(...) calls — since_date defaults to nil → backward compatible

Behavioral note (for support / release notes)

This bounds family-level post-sync auto-matching to the last 90 days. One consequence worth flagging: any cross-account transfers that are still unmatched and older than 90 days at the time this ships won't be auto-matched going forward — e.g. a user whose accounts were briefly disconnected, with transfers during that gap, would need to match those manually. The account-level path keeps full history (since_date: nil), so new connections and historical imports are unaffected; this only concerns already-existing, still-unmatched transfers older than 90 days. Worth a line in release notes so support is aware.

Edge case (practically a non-issue): the since_date filter keys off inflow_candidates.date, so a transfer whose two sides straddle the exact 90-day boundary could have one side excluded. Since the match window is ≤7 days, this only occurs for a transfer landing right at the boundary.

Test plan

  • test/models/family/auto_transfer_matchable_test.rb — 19 tests, 0 failures
  • test/models/family_test.rb — 17 tests, 0 failures
  • test/models/family/syncer_test.rb — 3 tests, 0 failures
  • Verify Sentry SURE-APP-WX resolves after deploy

Family::Syncer#perform_post_sync called auto_match_transfers! with no
date filter, causing a full cartesian scan across ALL entries in a
family's accounts on every sync. For families with 10,000+ entries this
produced a slow self-join query that Sentry flagged 18,894 times
(SURE-APP-WX), last seen today.

The fix adds a since_date parameter to transfer_match_candidates and
auto_match_transfers!. At the family level (nightly post-sync), it now
limits the scan to entries dated within the last 90 days. Account-level
syncs (account.family.auto_match_transfers!(account: account)) keep
since_date: nil for full-history matching on a specific account.

Transfer pairs where one side falls outside the 90-day window are still
covered by the account-level sync path that fires when individual
provider accounts sync, which already scopes by account_id.

Fixes SURE-APP-WX
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

auto_match_transfers! and transfer_match_candidates gain a since_date: nil keyword argument. When provided, both SQL UNION ALL branches filter inflow candidates to dates on or after that value. The post-sync step in Family::Syncer now passes since_date: 90.days.ago.to_date to scope matching to the last 90 days.

Changes

Date-scoped auto-transfer matching

Layer / File(s) Summary
since_date parameter and SQL filtering
app/models/family/auto_transfer_matchable.rb
transfer_match_candidates and auto_match_transfers! each gain a since_date: nil keyword. Both UNION ALL SQL branches add (:since_date IS NULL OR inflow_candidates.date >= :since_date) to candidate filtering conditions.
Syncer post-sync wiring
app/models/family/syncer.rb
perform_post_sync now calls family.auto_match_transfers! with since_date: 90.days.ago.to_date, replacing the prior no-argument call.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

A bunny hops back just 90 days,
No need to match through ancient haze.
since_date arrives with a gentle nil,
The SQL predicate bends to our will.
🐇 Fresh transfers matched, old ones still!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding a performance optimization via date-bound transfer matching to the sync process.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@jjmata jjmata left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Clean and well-scoped. The since_date filter is correctly applied in both arms of the UNION ALL in the SQL, so the window is symmetric.

One thing to be aware of: any unmatched transfers older than 90 days that exist at the time this merges will never be auto-matched going forward. Users who had a sync gap (e.g., accounts briefly disconnected) with transfers during that gap will need to match them manually. That may be acceptable, but it's worth a note in the PR description or a brief callout in release notes so support is aware.

Also worth confirming: the since_date filter is based on inflow_candidates.date in both UNION branches. If a transfer's two sides have dates 1 day apart straddling the 90-day boundary, one candidate could be excluded. Given date_window is typically ≤7 days, this edge case would require a transfer to be submitted right at the 90-day mark, so practically it's a non-issue — just good to be aware of.


Generated by Claude Code

@JSONbored

Copy link
Copy Markdown
Contributor Author

Thanks @jjmata — both are awareness items rather than code changes, so I added a Behavioral note (for support / release notes) section to the PR description capturing them:

  • Pre-existing >90-day unmatched transfers: documented that anything still unmatched and older than 90 days when this ships won't be auto-matched going forward (e.g. a sync-gap user with transfers during the gap would match manually), and that the account-level path keeps full history so new connections/imports are unaffected. Flagged it for release notes so support is aware.
  • Boundary edge case: noted it too — the filter keys off inflow_candidates.date, so a transfer whose sides straddle the exact 90-day boundary could have one side excluded; with the ≤7-day match window this only happens right at the boundary, so practically a non-issue, as you said.

No code change needed for either. Repo has no CHANGELOG file, so the PR description is the home for the note (happy to mirror it into release notes if there's a process for that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: SyncJob transfer matching does full-table scan on every family sync

2 participants