Overall the structure is solid — the two-layer worker design, the sync_with_virtuous flag, and the cutoff-based idempotency are all reasonable approaches. But there are real issues worth addressing before merge.
1. mark_gifts_synced uses updated_at, not the reversal timestamp
mark_gifts_synced filters by updated_at < cutoff, but a reversed gift was selected because refunded_at/chargedback_at/bounced_at < cutoff. If a gift's updated_at was bumped after the reversal (e.g. by some unrelated update), the gift gets sent to Virtuous but virtuous_synced_at doesn't get set, so it'll be re-queued and sent again next run. Virtuous should deduplicate on transactionId, but this is still wasteful and fragile. The reversal's virtuous_synced_at should be compared to the relevant *_at field, not updated_at.
2. Redundant DB query per batch in queue_gift_syncs
Line 31 of virtuous_batch_sync_worker.rb does:
'account_ids' => Gift.where(id: batch_ids).select_map(:account_id).uniq,But account_id was already fetched in received_gift_rows / reversed_gift_rows (which select(:id, :account_id)). The code builds a batch_ids array and then re-queries the DB to get back the same account_ids that were already in memory. This should just slice from the already-loaded data.
3. first_gift_in_partnership? fires a query per gift
Gift.where(partnership_id:).exclude(received_at: nil).order(:received_at).get(:id) == idThis is called inside virtuous_transaction, which is called inside a .map on a batch of gifts. 100 gifts in a batch = 100 queries. For a nightly sync processing a lot of partnerships this will be slow. The batch worker already has all the gift IDs in memory — this could be resolved by pre-computing the first-gift-per-partnership as a set before mapping.
4. N+1 queries when building contact payloads
virtuous_contact_payload_for_contacts_endpoint touches: officer, email_lists, tags_dataset, address, and spouse — none of which are eager loaded. sync_contacts just does Account.where(id: account_ids).all with no associations. With 100 accounts per batch that's potentially 500+ queries. Same problem in the gifts path for virtuous_contact_payload_for_gifts_endpoint and intermediary.account for soft gifts.
5. The sync_with_virtuous flag propagation is spread across three models
The logic for setting/unsetting sync_with_virtuous lives in:
Gift::Virtuous#after_create(gift received → set flag on account)Account::Virtuous#after_update(flag set → propagate to spouse)Relationship#sync_virtuous_flag_on_spouse_create(new spouse → propagate)Relationship#unsync_virtuous_flag_on_spouse_destroy(spouse removed → maybe unset)
This is hard to reason about and easy to break. In particular: the after_update hook propagates to spouse when sync_with_virtuous changes to true, which then fires after_update on the spouse... which checks spouse.sync_with_virtuous again before propagating. It avoids infinite loops but only because the spouse's flag is already true by then. This is subtle and fragile. A comment explaining the intended invariant would help.
6. needing_virtuous_contact_sync will catch every account update forever
The query is:
where(sync_with_virtuous: true)
.where { updated_at < cutoff }
.where { (virtuous_synced_at =~ nil) | (virtuous_synced_at < updated_at) }This is correct for catching accounts that changed since their last sync. But it means every time any column on an account changes — including things like last_gave_at, customerio_quality, internal counters — it re-syncs to Virtuous. Most of those changes are irrelevant to what Virtuous cares about. This will cause a lot of unnecessary API calls over time and make it hard to know why a contact was re-synced.
7. Commented-out createImport: true
# createImport: true,Left in import_gifts. Needs a decision: is it intentionally off, or WIP? If intentionally off, remove it with a comment explaining why. Leaving dead commented code in an integration like this will cause confusion later.
8. The comment says "10 PM Central" but the cutoff drifts with DST
CUTOFF_HOUR_UTC = 4 is 10 PM Central in winter (CST, UTC-6) but 11 PM Central in summer (CDT, UTC-5). The comment in VirtuousBatchSyncWorker says "10 PM Central" — this is only accurate half the year. Not a bug (the cutoff is always 4 AM UTC), but if the intent is "don't include gifts that arrived in the last business day's final hour," coverage will vary seasonally by an hour.
9. No handling of TooManyRequests in the workers
When Virtuous returns 429, Virtuous::TooManyRequests is raised and propagates up through VirtuousSyncWorker#perform. Sidekiq will retry using its standard exponential backoff (first retry ~15s, then minutes, then tens of minutes). It won't respect a Retry-After header. This is probably acceptable for now, but once volume is high enough the nightly sync could consistently hit rate limits and start degrading.
10. The soft gift passthroughContact direction should be verified against Virtuous
For soft gifts:
contact=intermediary.account(e.g. the DAF custodian)passthroughContact=account(the donor)
The intent seems to be: "the gift came from the DAF, but credit the donor." Worth confirming with Virtuous's documentation or support — some CRMs use these fields the opposite way (contact = the donor, passthrough = the intermediary). Getting this backwards means the gift in Virtuous is credited to the wrong person.
11. retries ||= 1 starts at 1, not 0
In with_error_handling, the retry counter initializes to 1. So the sleep schedule is sleep(4), sleep(9), sleep(16) — not sleep(1), sleep(4), sleep(9). Still reasonable, but any "up to 4 retries" framing is misleading — it's 3 retries (4 total attempts).
12. parse_body silently returns {} on JSON errors
If Virtuous returns non-JSON (e.g. an HTML error page), the caller gets {} with no indication anything went wrong. Should at minimum be logged.
13. Missing index on gifts.virtuous_synced_at
The reversed_needing_sync query compares virtuous_synced_at against refunded_at, chargedback_at, and bounced_at. There's an index on accounts.sync_with_virtuous but no index on gifts.virtuous_synced_at. Once there are millions of gifts this query may be slow.
- The two-layer worker design (batch orchestrator + per-batch sync worker) is the right call — keeps the clock job fast and makes individual failures recoverable via Sidekiq retry.
- Re-fetching gifts inside
VirtuousSyncWorkerby ID (rather than trusting the batch worker's snapshot) is smart — handles status changes between enqueue and execution. - The
reversed_athelper inGift::Statusis reused correctly. - The cutoff being passed through from batch → sync worker prevents the window problem where a long-running sync picks up records that changed mid-run.
- Excluding
intermediarygifts from sync via thetypesarray is the right place to do it.