Repository navigation
Pass ctx to GORM connection, handle creating instances and ensure we omit associations - #926
Merged
gabriel-samfira merged 3 commits intoOct 22, 2026
Conversation
No query in the sql package carried a context. Every call site used s.conn directly and most methods accepted a context only to discard it. A query whose server never answered would park the calling goroutine for the life of the process, with no deadline and no way to cancel it. Pass the caller's context to gorm on every query and add a context to the ControllerStore methods that had none. Add statement_timeout, lock_timeout and connect_timeout to the PostgreSQL DSN as a backstop for callers that still pass a context with no deadline. Schema migrations now run before any runtime connection exists, over connections they open and close themselves, with those timeouts disabled. An AutoMigrate on a large table can legitimately outrun them, and being aborted half way through leaves the schema in an unknown state. The provider worker also bounds its own store calls. The instance manager gets 30 seconds per call and the bulk loads get two minutes, since those run with the worker mutex held and stall every scale set rather than one instance. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
An instance in creating has no owner once the create call returns without recording an outcome. consolidateState had no case for it, the scale set reconcile passes skip it, the transition table allows only error and running, and DeleteRunner refuses it. The only way out was a restart. Reaching that switch proves no create is in flight, since consolidation is serialized and runs on the same goroutine that performs the create. Fail the instance and let the existing error lane delete it. The write is not forced on purpose. If the runner booted and registered anyway the row is already running, which does not allow a move to error, so the state machine refuses us instead of killing a live runner. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
gorm saves associations by default and the store preloads them, so a write to one row also wrote several other tables. updateInstance locks the instance row and then upserted the parent scale set, while SetScaleSetLastMessageID locks the scale set row and then upserted every runner row and the owning entity. Each one holds what the other wants, which PostgreSQL reports as a deadlock (SQLSTATE 40P01). Even where the cycle does not close, a runner status write had to wait on any uncommitted update of its scale set row. That is how a runner ends up stuck in pending_create or creating while a scale set update holds that row across a forge call. updateScaleSet already carried an Omit for this. The two listener checkpoint writes had none, and updatePool omitted the owner and the tags but not Instances. Use Omit(clause.Associations) everywhere rather than a list of names, since the list is what let Instances slip through. The create paths set the foreign key explicitly, because gorm is what copies the parent key across and sqlToParamsInstance reads ProviderName off the association. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
3 of 12 tasks
cbartz
added a commit
to canonical/github-runner-operators
that referenced
this pull request
Oct 25, 2026
Pin GARM to the merge of cloudbase/garm#926. It bounds every PostgreSQL query with a statement, lock and connect timeout, reaps runners left in creating when recording them as running fails, and omits associations when writing instances and scale sets to remove an ABBA deadlock between those updates.
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
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.
This change fixes a number of issues in the database layer:
pending_create. TheconsolidateState()function then picks it up and marks it ascreatingbefore calling the provider. When the provider returns, we mark the instance asrunning. If the call to the database to mark the instance asrunning, fails, we cleanup the runner form the IaaS, but nothing ever reaps the runner stuck in creating. Thic change addresses that scenario.updateInstance(), we lock the instance for update, and because we never omitted associations, we update the scaleset, then we update the actual instance. Then inSetScaleSetLastMessageID(), we lock the scaleset for update, we update the instance associated with the scale set and then we update the scaleset. So both touch the same tables and each locks the respective row it wants to edit, but in reverse order than the other function, leading to an ABBA deadlock. The last commit omits all associations from the offending functions.