Skip to content

fix(jobs): delegate recurring-transaction sync gate to Sync.for_family#1975

Open
galuis116 wants to merge 2 commits into
we-promise:mainfrom
galuis116:fix/recurring-transactions-sync-gate-via-reflection
Open

fix(jobs): delegate recurring-transaction sync gate to Sync.for_family#1975
galuis116 wants to merge 2 commits into
we-promise:mainfrom
galuis116:fix/recurring-transactions-sync-gate-via-reflection

Conversation

@galuis116
Copy link
Copy Markdown

@galuis116 galuis116 commented May 25, 2026

Summary

IdentifyRecurringTransactionsJob#family_has_incomplete_syncs? (app/jobs/identify_recurring_transactions_job.rb:45-60) was the debounce gate that delayed RecurringTransaction::Identifier until in-flight provider syncs settled. The gate hand-rolled the list of provider *_items associations it polled — plaid_items, simplefin_items, lunchflow_items, enable_banking_items, sophtron_items — and silently missed nine other Syncable provider concerns on Family (coinbase_items, binance_items, kraken_items, coinstats_items, snaptrade_items, mercury_items, brex_items, indexa_capital_items, ibkr_items). When a sync on any of those nine was in flight, the gate fell through, the identifier ran against a partial dataset, and the resulting RecurringTransaction row got a stale occurrence_count; the late-arriving sync then re-scheduled the job and the follow-up run upserted via find_or_initialize_by inheriting the stale row.

The maintainers' own Sync.for_family (app/models/sync.rb:61-75) already enumerates every *_items association via Family.reflect_on_all_associations(:has_many) filtered by inclusion of the Syncable module — exactly the helper this gate should delegate to so the list cannot drift again. The file has had a single edit since creation (Apr 19, 2026, #591 bolting on Sophtron as the 5th entry); this PR closes the regression class so the next provider integration doesn't reopen it.

Fixes #1974.

Fix

  • Add Sync.any_incomplete_for?(family) class method on Sync that wraps for_family(family).incomplete.exists? — mirrors the existing Sync.clean / Sync.for_family class-method idiom and is reusable from any future caller that needs the same gate.
  • Rewrite IdentifyRecurringTransactionsJob#family_has_incomplete_syncs? to delegate. 14 lines → 1, plus a docstring explaining the drift the delegation closes.

The reflection-based discovery in family_syncable_associations (sync.rb:81-88) was already trusted by Sync.for_family itself — same contract, same trust gate, no new abstraction.

Tests

New file: test/jobs/identify_recurring_transactions_job_test.rb (5 tests)

  • In-flight Coinbase sync → identifier is not called (would fail today; gate didn't poll coinbase_items)
  • In-flight Mercury sync → identifier is not called (would fail today)
  • No in-flight syncs → identifier is called (happy path)
  • Missing family → no-op (existing guard)
  • Superseded-by-newer-schedule → no-op (existing guard)

Added to test/models/sync_test.rb (2 tests)

  • Sync.any_incomplete_for? returns true for an in-flight Mercury-item sync; flips to false once the sync completes
  • Sync.any_incomplete_for? returns true for a sync directly on the Family row

The existing test for_family includes syncable provider item associations from family reflections (sync_test.rb:217) was the precedent that reflection-based discovery is the right pattern; the new tests extend that same shape to the incomplete predicate.

Files changed

  • app/jobs/identify_recurring_transactions_job.rb — delegate the gate; 14-line hand-rolled list deleted, replaced with 1-line call.
  • app/models/sync.rb — new Sync.any_incomplete_for? class method (8 lines incl. docstring).
  • test/jobs/identify_recurring_transactions_job_test.rb — new file, 5 tests.
  • test/models/sync_test.rb — 2 new tests pinning Sync.any_incomplete_for?.

CI / pre-PR checks

The standard CI gate from CLAUDE.md (bin/rails test, bin/rubocop -f github -a, bin/brakeman --no-pager) is required. No *.erb changed (erb_lint no-op). No API endpoint changed (rswag regeneration not required).

[Allow edits from maintainers] is enabled on this PR.

Summary by CodeRabbit

  • Refactor

    • Unified internal sync-checking used by recurring transaction identification to more reliably detect incomplete syncs across a family and its linked provider items.
  • Tests

    • Added tests for the recurring transactions job covering in-flight provider syncs, missing families, and cache-based scheduling suppression.
    • Added tests validating the new unified sync-detection behavior.

Review Change Stack

`IdentifyRecurringTransactionsJob#family_has_incomplete_syncs?` hand-rolled
the list of provider `*_items` associations it polled — plaid, simplefin,
lunchflow, enable_banking, sophtron — missing nine other `Syncable`
provider concerns on `Family`: coinbase, binance, kraken, coinstats,
snaptrade, mercury, brex, indexa_capital, ibkr. When a sync on any of those
nine was in flight, the debounce gate fell through and
`RecurringTransaction::Identifier` ran against a partial dataset; the
follow-up re-enqueue then hit the `find_or_initialize_by` upsert path and
inherited the stale `occurrence_count`. Same drift pattern that bolted
sophtron on as the 5th entry (we-promise#591) was already an iteration of.

The maintainers' own `Sync.for_family` (sync.rb:61) already enumerates every
`*_items` association via `Family.reflect_on_all_associations(:has_many)`
filtered by inclusion of `Syncable` — exactly the helper the gate should
delegate to so the list cannot drift again.

- Add `Sync.any_incomplete_for?(family)` class method that wraps
  `for_family(family).incomplete.exists?`.
- Rewrite `family_has_incomplete_syncs?` to delegate. 14 lines → 1.
- New test file `test/jobs/identify_recurring_transactions_job_test.rb`
  covers in-flight Coinbase + Mercury (gate fires), idle (identifier runs),
  missing family, and superseded-by-newer-schedule.
- `test/models/sync_test.rb` gets 2 new tests pinning
  `any_incomplete_for?` against a provider `_items` sync and a
  family-itself sync.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@superagent-security superagent-security Bot added contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis. labels May 25, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 96e9166e-d041-41fe-bf49-c7aef9b05f76

📥 Commits

Reviewing files that changed from the base of the PR and between b7a3574 and 630a5b0.

📒 Files selected for processing (1)
  • test/jobs/identify_recurring_transactions_job_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/jobs/identify_recurring_transactions_job_test.rb

📝 Walkthrough

Walkthrough

Replaces IdentifyRecurringTransactionsJob's hand-rolled incomplete-sync checks with a delegation to new Sync.any_incomplete_for?(family). Adds unit tests for the Sync method and job tests verifying debounce behavior for various in-flight provider syncs, missing family, and cache superseding.

Changes

Sync debounce gate unification

Layer / File(s) Summary
Sync.any_incomplete_for? method
app/models/sync.rb
New method that returns whether a family has any incomplete syncs via Sync.for_family(family).incomplete.exists?.
Job debounce gate refactor
app/jobs/identify_recurring_transactions_job.rb
Replaces family_has_incomplete_syncs?'s hand-rolled provider association checks with a single call to Sync.any_incomplete_for?(family).
Unit tests for Sync.any_incomplete_for?
test/models/sync_test.rb
Adds tests asserting the method is true when provider-item or family syncs are incomplete and false after completion.
Integration tests for job debounce behavior
test/jobs/identify_recurring_transactions_job_test.rb
Adds tests verifying the job skips when provider syncs (Coinbase/Mercury) are in flight, runs when none are incomplete, skips for missing family, and respects cache-based superseding.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • we-promise/sure#1635: Related changes touching Sync.for_family / family-scoped sync querying logic used by the new Sync.any_incomplete_for?(family).

Suggested reviewers

  • jjmata

Poem

🐰 A gate once checked just five, and missed the rest in line,
Now reflection calls them all — each syncable, in time.
Tests hop in, the gate holds fast, no partial-counts remain,
The garden hums in tidy rows, recurring patterns sane. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main refactoring: delegating the sync gate logic to Sync.for_family, which is the core fix for the regression.
Linked Issues check ✅ Passed All objectives from issue #1974 are met: the hand-rolled list is replaced with Sync.any_incomplete_for?(family), which delegates to Sync.for_family using reflection-based discovery, and comprehensive tests verify both missing-provider cases and the predicate.
Out of Scope Changes check ✅ Passed All changes are tightly scoped to fixing the regression: the job refactor, new Sync method, and tests directly address issue #1974; no unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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 and usage tips.

Copy link
Copy Markdown
Collaborator

jjmata commented May 26, 2026

Clean and correct refactor. A few notes:

Root cause: The hand-rolled list covering only 5 of 14 Syncable provider associations was a classic "update the list in one place but not the other" maintenance hazard. Delegating to Sync.for_family (which uses Family.reflect_on_all_associations + Syncable module check) is the right fix — new provider integrations are automatically covered.

Sync.any_incomplete_for?: A natural addition to Sync's class-method API. Wrapping for_family(family).incomplete.exists? in a named method makes the intent clear at the call site and keeps the debounce gate in the job a one-liner.

Tests: The Coinbase test (creates a coinbase_item directly rather than from fixtures, since it's one of the previously-uncovered providers) and the Mercury test together verify both the bug and the happy path. The cache-based scheduling gate test and missing-family guard cover the existing defensive paths without changing their behavior.


Generated by Claude Code

… test env)

`Rails.cache` is `ActiveSupport::Cache::NullStore` in the Rails test env, so
the previous test's `Rails.cache.write(cache_key, @scheduled_at + 10, ...)`
was a no-op and `Rails.cache.read(cache_key)` returned `nil`. The
supersession short-circuit `return if latest_scheduled && latest_scheduled
> scheduled_at` then fell through, the job proceeded to invoke
`RecurringTransaction::Identifier`, and the Mocha
`.expects(:identify_recurring_patterns).never` failed in CI.

Switch to `Rails.cache.stubs(:read).with(cache_key).returns(...)` — the
same idiom `test/models/provider/twelve_data_test.rb:186-197` already uses
for the cache layer. Add an `assert_nil` on the bare `perform` return so
Minitest's assertion counter sees an explicit assertion (silences the
"missing assertions" warning).

No production-code change. Behavior under test is unchanged; only the test
mechanism for simulating "newer scheduled run already in cache" is fixed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis.

Development

Successfully merging this pull request may close these issues.

Bug: IdentifyRecurringTransactionsJob debounce gate misses 9 of 14 Syncable provider associations

2 participants