Make instance state transitions race-tolerant and bound provider execution time#817
Merged
gabriel-samfira merged 13 commits intoJul 8, 2026
Merged
Conversation
When a scale set job message arrives for a runner that another code path
(reaper, consolidate, provider) has already driven into the deletion lane,
or that is already terminal, UpdateInstance correctly refuses the status
transition. Previously HandleJobsCompleted/HandleJobsStarted treated that
as fatal, which wedged the message listener on the poisoned message and
blocked every subsequent job assignment for the scale set.
Add typed transition errors that carry the from-status observed inside the
update transaction:
- InstanceTransitionError (garm-provider-common/errors)
- RunnerTransitionError (internal/errors)
validate{Instance,Runner}StatusTransition now return these instead of a
bare BadRequestError; they still map to HTTP 400 and keep the same message.
The handlers use errors.As plus InstanceIsBeingDeleted / RunnerIsTerminal to
recognise a transition whose intent is already satisfied and continue, while
any other error still stops processing.
Deciding on the tx-time from-status carried by the error (rather than a
post-error re-fetch) is race-free: the deletion lane is monotonic, so a
status observed there cannot revert.
Extend the transition-error tolerance beyond the scale-set job handlers to
the remaining UpdateInstance callers that can race a concurrent deletion or
reap:
- scaleset consolidateRunnerState and scale-down removal (pending_delete):
a runner another path already moved into the deletion lane no longer
logs a spurious error / aborts the reconcile; it is counted as removed.
- the delete-runner path (runner.go): if the instance is already being
deleted, the caller's intent is met, so return nil instead of erroring.
- the pool workflow-job-completed handler (pool.go): the pool twin of
HandleJobsCompleted, same pending_delete race.
- the runner status callback (AddInstanceStatusMessage): a status reported
by an already-terminal runner is moot, so tolerate it.
All decisions key off the from-status carried by the typed transition error
(InstanceTransitionError / RunnerTransitionError), not a re-fetch, and only
the benign already-satisfied case is tolerated; other errors still stop.
GitHub assigns jobs based on its own registration state. The only code path that transitions a runner to idle in the database is the runner's own status callback, so a scale set job started message can arrive while the database still has the runner in pending or installing. The message raced the idle callback, or the image never sends callbacks at all. Refusing the transition wedged the scale set listener. The message was redelivered forever and blocked every subsequent message for that scale set. A job started event is proof that the runner finished installing, so model that in the state machine instead of special-casing the handler. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
A stalled provider create can outlive a runner's entire lifecycle. The JIT registered runner boots, runs a job and completes it before CreateInstance returns, so the instance is still in creating and cannot transition to pending_delete. Treating this as a fatal error wedged the scale set message queue for the remainder of the create call. Skip the update instead. Once the instance leaves the creating state, the runner is gone from github and consolidation will move the instance to pending_delete. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
The transition tolerance branches released their locks with the remove flag set, which deletes the lock map entry. Unlock has no ownership check and the delete is unconditional. A goroutine blocked in Lock() on the same entry wakes up holding the old mutex, while the next TryLock creates a fresh entry and also succeeds. Two goroutines then believe they hold the same runner's lock. These branches fire exactly when another code path is working on the runner, so keep the entry and only release the lock. The deletion path removes the entry once the record is actually gone. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
The setRunnerDBStatus function swallowed ErrNotFound and returned a zero value instance with a nil error. Both call sites then wrote the zero value instance back into the worker's runner cache, corrupting it. Propagate the error and let each caller decide what to do. A single runner's update error also aborted consolidation for the entire scale set, repeating every tick. Log the error and continue with the remaining runners instead. Instances in creating are now skipped when cross checking against github, as they cannot transition to pending_delete while the provider create call is in flight. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
A failed provider delete moves an instance from deleting back to error, so the deletion lane is not strictly monotonic. Tolerating a status observed there is still safe, because reconciliation re-drives the instance to pending_delete until the delete succeeds. Reword the comments so nobody builds on the stronger guarantee. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
The provider versus database sweep in consolidateRunnerState returned early if removing one runner failed, skipping the remaining runners and repeating every consolidation tick. Log the error, release the runner's lock and continue with the rest, the same as the reaper and the github cross check already do. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
A hung provider binary froze its instance in a transient state indefinitely. The exec call had no deadline, the instance manager holds its mutex for the duration of the call and nothing else can transition an instance out of creating. The only way out was restarting GARM. This change adds an optional exec_timeout_seconds setting to the external provider config. When set, every invocation of the provider binary is bounded by it. On expiry the child process is killed and the operation returns an error. For instance creation, the existing failure path marks the instance as error and cleans it up. The default of 0 preserves current behavior. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
A create that outlives the pool or scale set bootstrap timeout can only produce a runner that is already considered failed, and while the create is in flight the instance sits in creating, where reaping cannot touch it. Wrap every CreateInstance call in a context deadline derived from the entity's bootstrap timeout. The provider level exec timeout still applies to all operations. For creates, the effective deadline is whichever is sooner, since nested contexts keep the earlier deadline. On expiry the binary is killed and the existing create failure path takes over. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
The instance locks are worker local. They serialize the scale set worker's own goroutines. The provider worker does not participate in instance locking; it reacts to status transitions via watcher events as soon as they commit. Several comments claimed otherwise, or deferred cleanup of interrupted creates to the wrong owner. Reword them to describe the actual mechanisms. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
The new error handling branches pushed consolidateRunnerState over the gocyclo threshold. Move the provider cross check into its own function. No functional change. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
When agent mode is used, a runner status is set to offline by the agent until the runner process is started by the agent. This is an expected transition. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Several code paths (listener, reaper, consolidation, provider worker, API) race each other when updating instance state. A code path that lost a benign race treated the refused transition as fatal. In prod this wedged scale set listeners: a job message for an already-deleted runner was redelivered forever, blocking all messages for that scale set.
This change keeps the state machine strict, but lets callers see why a transition was refused via typed errors carrying the from-status. Callers now tolerate only refusals whose intent is already satisfied (runner already being deleted or already terminal). Everything else still fails loudly.
Also included:
Note: Instance creation is now bounded by runner_bootstrap_timeout (default 20 minutes). If your provider legitimately needs longer, raise the bootstrap timeout on the affected pools/scale sets.
Specific transition error types were added to garm-provider-common and the dependency updated in this branch.
Supersedes #812