scaleset: tolerate invalid instance status transitions in job message handlers#812
Conversation
… handlers HandleJobsCompleted and HandleJobsStarted only tolerated ErrNotFound when updating an instance's status/runner_status in response to a scale set job message. If the instance had already transitioned to a terminal state (e.g. "deleted", because the reconciler noticed it went missing from the provider/github after a host failure) by the time a delayed "job started"/"job completed" message arrived, UpdateInstance would reject the transition with a BadRequestError. That error was treated as fatal, causing handleSessionMessage to return before advancing the message cursor or deleting the message. Since the same poisoned message would then be redelivered forever, the entire scale set listener would get stuck retrying it, blocking every subsequent message (including new job assignments) for that scale set. Observed in prod: two scale sets stuck in a tight retry loop logging "invalid instance status transition from deleted to pending_delete" hundreds of times, with a large backlog of queued jobs and idle runners that could never receive them. Treat BadRequestError from UpdateInstance the same way ErrNotFound is already treated: log and move on, since it means some other codepath already resolved the runner's terminal state and there's nothing left for us to do.
| locking.Unlock(job.RunnerName, true) | ||
| continue | ||
| } | ||
| if errors.Is(err, runnerErrors.ErrBadRequest) { |
There was a problem hiding this comment.
This is interesting. The HandleJobsStarted() function is called as a result of a message that comes over the scaleset longpoll session and only happens when a runner transitions from idle to active. While the runner is active, the github API does not allow the runner to be removed.
A reap operation usually tries to remove the runner from github first, then marks the runner as pending_delete. The runners should then be cleaned up from the provider. Unless I messed up and didn't do that, there should be no way a runner is reaped by GARM while active. If a runner is forcefully removed from the provider while idle, it goes offline and cannot pick up a job. So it can't transition to active and a message that triggers HandleJobsStarted() won't arrive.
If a runner is forcefully removed from the provider while active it times out after 10 minutes, but then a completed status is sent and a different function should fire.
So I am unsure how this code path was triggered in your env. It may be a bug somewhere else, and I would like to track it down and fix the cause rather than treat the symptom.
Could you share some debug logs by any chance that tracks a runner that has hit this? Are you using agent mode? Were any custom changes made to GARM that could impact this?
| locking.Unlock(job.RunnerName, true) | ||
| continue | ||
| } | ||
| if errors.Is(err, runnerErrors.ErrBadRequest) { |
There was a problem hiding this comment.
This could indeed race if agent mode is used, but when in agent mode, the runner is set in pending_delete and eventually reaped. A retry here would hit ErrNotFound and should just continue. But we could find out more from logs if it you could share some (anonymized). You can find me on slack and we can debug this in private if it helps.
2 participants
HandleJobsCompletedandHandleJobsStartedonly toleratedErrNotFoundwhen updating an instance's status/runner_status in response to a scale set job message. If the instance had already transitioned to a terminal state (e.g.deleted, because the reconciler noticed it went missing from the provider/GitHub after a host failure) by the time a delayed "job started"/"job completed" message arrived,UpdateInstancewould reject the transition with aBadRequestError.That error was treated as fatal, causing
handleSessionMessageto return before advancing the message cursor or deleting the message. Since the same poisoned message would then be redelivered forever, the entire scale set listener would get stuck retrying it, blocking every subsequent message (including new job assignments) for that scale set.Observed in our prod cluster: two scale sets stuck in a tight retry loop logging
invalid instance status transition from deleted to pending_deletehundreds of times, with a large backlog of queued jobs and idle runners that could never receive them.This change treats
BadRequestErrorfromUpdateInstancethe same wayErrNotFoundis already treated: log and move on, since it means some other codepath already resolved the runner's terminal state and there's nothing left for us to do.