fix: don't unnecessarily restart scheduler #689
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The scheduler ensures only one run on each workspace is permitted to be running (the exception to this rule are "plan-only" runs, which we won't discuss here). A run begins in the
pending
state. It cannot transition to the next state,plan_queued
, until:(a) any other pending runs created before it have finished.
(b) its workspace is unlocked.
Once both conditions are true, the scheduler does the following:
(a) locks the workspace
(b) updates the status of the run to
plan_queued
(c) sets the run as the "current run" of the workspace
Once a run finishes it is also responsible for unlocking the workspace once a run completes. One of the bugs identified by the user is a race condition that occurs whenever a run finishes and its workspace is immediately deleted (this is not an untypical scenario, where you're testing changes on an "ephemeral" workspace): the scheduler receives the "run completed" event and often by this time the workspace has already been deleted, but the scheduler isn't aware of that, and it tries to unlock the workspace and it receives an error. This shouldn't be an issue because the error is "workspace not found" and the scheduler should understand that that means the workspace has since been deleted, no action need be taken and to move on. But instead it errantly interpets it as a transient error, and backs off and retries. The fix here is clear.
Another race condition occurs when the "run completed" event is received after the "workspace deleted" event. The scheduler processes the latter event and deletes its cached workspace accordingly. It then receives the "run completed" event and tries to lookup its workspace in its cache and it cannot find it. It reports this as an error to the user, and moves on. The "fix" here is to either accept this as an entirely reasonable race condition and suppress the error message; or to make a change to ensure events are processed in order. In this case I've opted for the former.
#685