Add scheduled job to sync all accounts every 24 hours #330
Conversation
Signed-off-by: Nikhil Badyal <[email protected]>
WalkthroughAdds a new daily scheduled job (02:22) and a corresponding ActiveJob that iterates all Family records, enqueues per-family async syncs, logs start/end and per-family errors, and adds the sidekiq-unique-jobs gem for Sidekiq locking support. Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as Scheduler (cron @ 02:22)
participant Job as SyncAllJob
participant DB as Family DB
participant Queue as Worker Queue
Cron->>Job: trigger SyncAllJob.perform
Note right of Job `#DDEBF7`: log "start"
Job->>DB: Family.find_each
loop per family
DB-->>Job: yield family
alt enqueue success
Job->>Queue: family.sync_later (enqueue async)
Queue-->>Job: ack enqueued
else error
Job-->>Job: log error (continue)
end
end
Note right of Job `#E6F4EA`: log "finished"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/jobs/accounts/sync_all_job.rb (2)
4-8: Consider adding error handling and observability.The current implementation lacks error handling and logging, which could make debugging issues difficult in production. Consider adding:
- Rescue blocks to handle potential failures gracefully
- Logging to track sync progress and errors
- Metrics/monitoring for observability
Apply this diff to add basic error handling and logging:
def perform + synced_count = 0 + error_count = 0 + Family.find_each do |family| - family.sync_later + begin + family.sync_later + synced_count += 1 + rescue => e + error_count += 1 + Rails.logger.error("Failed to enqueue sync for family #{family.id}: #{e.message}") + end end + + Rails.logger.info("Sync job completed: #{synced_count} families enqueued, #{error_count} errors") end
4-8: Monitor job performance and consider staggering syncs.Running this job every 30 minutes will synchronously enqueue sync jobs for all families at once. Depending on the number of families and downstream system capacity, this could:
- Create a thundering herd effect with all syncs starting simultaneously
- Take considerable time if you have many families
- Potentially overwhelm downstream APIs or databases
Consider:
- Adding metrics to track job execution time and family counts
- Implementing rate limiting or staggering (e.g., random delays) if you experience performance issues
- Monitoring queue depths and sync success rates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/jobs/accounts/sync_all_job.rb(1 hunks)config/schedule.yml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/jobs/accounts/sync_all_job.rb (1)
app/models/concerns/syncable.rb (1)
sync_later(14-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
|
Would love your thoughts as well, @sokie ... should we add any config in |
Signed-off-by: Juan José Mata <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/jobs/accounts/sync_all_job.rb (1)
5-7: Consider rate limiting for large family counts.If there are many families, enqueuing all sync jobs simultaneously could overwhelm the queue. Consider adding a delay between batches to spread the load.
Optional improvement:
def perform Family.find_each do |family| family.sync_later + rescue => e + Rails.logger.error("Failed to sync family #{family.id}: #{e.message}") end endOr with batch delays if scale is a concern:
def perform Family.find_each.with_index do |family, index| # Add 1 second delay per 10 families to spread load delay = (index / 10).seconds Accounts::SyncFamilyJob.set(wait: delay).perform_later(family) rescue => e Rails.logger.error("Failed to enqueue sync for family #{family.id}: #{e.message}") end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/jobs/accounts/sync_all_job.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/jobs/accounts/sync_all_job.rb (1)
app/models/concerns/syncable.rb (1)
sync_later(14-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
Maybe the only thing would be a checkbox to enable/disable. |
sokie
left a comment
sokie
left a comment
There was a problem hiding this comment.
While a setting would be nice, not a blocker to me.
jjmata
changed the title
jjmata
changed the title
|
I noticed a I’ve tried uncommenting that section and it appears a sync is performed daily (I still need to test it further). |
I can check tomorrow as well what's up |
|
Excellent! This PR might not be needed after all. |
|
Re-requested approval from @sokie so he can report back when he looks at this. |
|
@sokie I’m not sure if you tested it, but I noticed today when I first opened the app that a sync was performed in the background. I uncommented that bit in the It seems to be working even though I need to open the app. The approach in this PR is to automatically perform the sync in the background every X hours, if I properly understood, regardless of whether the user logs in or not. I’m not sure which approach you prefer. |
sokie
left a comment
sokie
left a comment
There was a problem hiding this comment.
@jjmata and @alessiocappa I've checked both.
The background sync job is better, the auto_sync.rb adds overhead to any request, needs user visit pages to trigger, and might cause spikes during heavy usage if multiple users are active. It is also limited to users family, while the background sync pulls in all families.
I would say to go ahead with this PR, and keep autosync commented, it's for the better
Checked up on this, and they turned it off here due to performance reasons reported here. |
OK, made a couple of small changes/changed it to every 24 hours at 2:22am ... will merge once it goes green. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/jobs/sync_all_job.rb(1 hunks)config/schedule.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/schedule.yml
🧰 Additional context used
🧬 Code graph analysis (1)
app/jobs/sync_all_job.rb (1)
app/models/concerns/syncable.rb (1)
sync_later(14-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
jjmata
changed the title
* Add scheduled job to sync all accounts every 30 minutes Signed-off-by: Nikhil Badyal <[email protected]> * Change job queue from default to scheduled Signed-off-by: Juan José Mata <[email protected]> * Flatten job into single directory * Every 30 minutes is a bit much and will trigger Sentry warnings * Locking and logging improvements * Add support for extra Sidekiq goodies --------- Signed-off-by: Nikhil Badyal <[email protected]> Signed-off-by: Juan José Mata <[email protected]> Co-authored-by: Nikhil Badyal <[email protected]>
Cherry-picked from here, thanks @nikhilbadyal!
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.