Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not execute removed cron workloads #17

Merged
merged 7 commits into from
Sep 4, 2024

Conversation

julik
Copy link
Contributor

@julik julik commented Sep 3, 2024

We need to check whether a workload was enqueued with a scheduler key, but no longer is in the cron table. If that is the case (we are trying to execute a workload which has a scheduler key, but the scheduler does not know about that key) it means that the workload has been removed from the cron table and must not run. Moreover: running it can be dangerous because it was likely removed from the table for a reason. Should that be the case, mark the job "finished" and return nil to get to the next poll. If the deployed worker still has the workload in its scheduler table, but a new deploy removed it - this is a race condition, but we are willing to accept it.

Note that we are already "just not enqueueing" that job when the cron table gets loaded - but it is not enough.
If our system can be in a state of partial deployment:

  [  release 1 does have some_job_hourly crontab entry ]
                 [  release 2 no longer does                           ]
                 ^ --- race conditions possible here --^

...and we remove the crontabled workloads during app boot, it does not give us a guarantee that release 1 won't reenqueue them. For example, via the "reinsert next scheduled" feature when the job is executing. This is why this safeguard is needed.

This protects us from a very dangerous failure mode where we would remove an entry from the cron table, deploy the change, and then still have the workload run a day later.

which Minitest would also assume to be a test case and flag, but since we are not using it anyway - just zap it
since folks usually care about the most recent changes
"string assignment" can be misleading
We need to double-check whether we are trying to execute a job which is not in our scheduler. It might be that keeping a table of those entries in the DB will be even safer later on.
We need to specify it both in the gemspec and in the standardrb config. Otherwise we use syntax features of Ruby 3.0 but we do not specify that our code actually would not load on the older Ruby version. And it is better to keep these two coordinated, so that standardrb alerts us if we use language features which our target Ruby version does not, in fact, support.
since that is the syntax we want to focus on
@julik julik changed the title Do not perform removed cron workloads Do not execute removed cron workloads Sep 3, 2024
Copy link
Collaborator

@svanhesteren svanhesteren left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks a lot, also for the cleanups.

@julik julik merged commit 06e829e into main Sep 4, 2024
1 check passed
@julik julik deleted the do-not-perform-removed-cron-workloads branch September 4, 2024 09:20
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.

2 participants