Fix cron jobs timing #463
Fix cron jobs timing #463
Conversation
This change improves the job enqueue logic specifically for CronJobs. Instead of using the current time for job queuing, it tries to calculates the actual scheduled time. This prevents potential issues where the job could be queued multiple times due to a delay in job execution. It also includes tests for the new run time calculation logic.
When a job is scheduled exactly between next and previous runs then round down to the previous run time. Updated specs to reflect this.
|
Is this a fix for jobs running multiple times if we use multiple machines? |
|
Yeah, it will solve most of the cases (see #181) — there might be some edge cases it might not though, and would require some locking mechanisms #332 (comment) |
|
Hi @marcelolx, Thanks for your reply. Could you go ahead and outline which edge cases you mentioned? I'm on the verge of deploying this and want to be 100% sure I'm doing things correctly. I will be running a 15-minute cron job. |
|
I don't have a specific one, but essentially, since we don't use any global lock mechanism to push a job to Redis, there is always the possibility that two different hosts could push the same job at the same time. That though, became very unlikely with this change. By global lock, I mean a lock that each host has to acquire to then push the job to the queue, something in similar to this #332 (comment) |
|
Hi @marcelolx , thanks for your reply. So I wonder, in an scenario where I have multiple sidekiq hosts but only have cron running on a single machine, should I worry about the message being executed multiple times by other hosts? |
|
@michelson no, no need to worry — if you run the scheduler on a single host it won't schedule it twice. With these changes here, it shouldn't run twice even if you run the scheduler on multiple hosts. |
Hey folks!
So I recently had some free time and decided to revisit this issue I had previously fixed at the company I was working for back in 2019.
I moved the calculation method into the
Utilsmodule to make testing easier and added lots of comments explaining what was going on. Added tests for the method since testing this issue through Rufus is incredibly difficult.Sorry @marcelolx for never getting back to you on this.
This would close #396 #276 and #455