From 000bc334c9da33b8d9ed749aa1a462c2532b9d0d Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Tue, 13 Jan 2026 15:04:51 +0000 Subject: [PATCH] fix: reuse reconciliation lock transaction for read operations in prebuilds (#21408) ## Description Reuses the reconciliation lock transaction for read operations during prebuilds reconciliation, reducing unnecessary database connections. ## Changes * Use the lock transaction (`db`) for read operations and `c.store` for write operations: * `GetPrebuildsSettings`: now uses `db` * `SnapshotState`: now uses `db` * `MembershipReconciler`: continues to use `c.store` (performs write operations) * Add comments explaining the transaction model and when to use `db` vs `c.store` Related to: https://github.com/coder/coder/pull/20587 --- enterprise/coderd/prebuilds/membership.go | 10 +++---- enterprise/coderd/prebuilds/reconcile.go | 36 +++++++++++++---------- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/enterprise/coderd/prebuilds/membership.go b/enterprise/coderd/prebuilds/membership.go index 47047ee608..8a8120d026 100644 --- a/enterprise/coderd/prebuilds/membership.go +++ b/enterprise/coderd/prebuilds/membership.go @@ -36,13 +36,13 @@ func NewStoreMembershipReconciler(store database.Store, clock quartz.Clock, logg // ReconcileAll ensures the prebuilds system user has the necessary memberships to create prebuilt workspaces. // For each organization with prebuilds configured, it ensures: -// * The user is a member of the organization -// * A group exists with quota 0 -// * The user is a member of that group +// * The prebuilds user is a member of the organization +// * A prebuilds group exists with quota allowance 0 (admins should adjust based on needs) +// * The prebuilds user is a member of that group // // Unique constraint violations are safely ignored (concurrent creation). -// -// ReconcileAll does not have an opinion on transaction or lock management. These responsibilities are left to the caller. +// ReconcileAll performs independent write operations without a transaction. +// Partial failures are handled by subsequent reconciliation cycles. func (s StoreMembershipReconciler) ReconcileAll(ctx context.Context, userID uuid.UUID, groupName string) error { orgStatuses, err := s.store.GetOrganizationsWithPrebuildStatus(ctx, database.GetOrganizationsWithPrebuildStatusParams{ UserID: userID, diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 2124044608..ee24cbbf57 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -271,22 +271,20 @@ func (c *StoreReconciler) Stop(ctx context.Context, cause error) { } } -// ReconcileAll will attempt to resolve the desired vs actual state of all templates which have presets with prebuilds configured. +// ReconcileAll attempts to reconcile the desired vs actual state of all prebuilds for each +// (organization, template, template version, preset) tuple. // -// NOTE: +// The result is a set of provisioning actions for each preset. These actions are fire-and-forget: +// the reconciliation loop does not wait for prebuilt workspaces to complete provisioning. // -// This function will kick of n provisioner jobs, based on the calculated state modifications. +// An outer read-only transaction holds an advisory lock ensuring only one replica reconciles at a time. +// This transaction remains open throughout the entire reconciliation cycle. Goroutines responsible for +// preset reconciliation use separate, independent write transactions (via c.store). In the rare case +// of the lock transaction failing mid-reconciliation, goroutines may continue while another replica +// acquires the lock, potentially causing temporary under/over-provisioning. Since the reconciliation +// loop is eventually consistent, subsequent cycles will converge to the desired state. // -// These provisioning jobs are fire-and-forget. We DO NOT wait for the prebuilt workspaces to complete their -// provisioning. As a consequence, it's possible that another reconciliation run will occur, which will mean that -// multiple preset versions could be reconciling at once. This may mean some temporary over-provisioning, but the -// reconciliation loop will bring these resources back into their desired numbers in an EVENTUALLY-consistent way. -// -// For example: we could decide to provision 1 new instance in this reconciliation. -// While that workspace is being provisioned, another template version is created which means this same preset will -// be reconciled again, leading to another workspace being provisioned. Two workspace builds will be occurring -// simultaneously for the same preset, but once both jobs have completed the reconciliation loop will notice the -// extraneous instance and delete it. +// NOTE: Read operations must use db (the lock transaction) while write operations must use c.store. func (c *StoreReconciler) ReconcileAll(ctx context.Context) (stats prebuilds.ReconcileStats, err error) { ctx, span := c.tracer.Start(ctx, "prebuilds.ReconcileAll") defer span.End() @@ -307,9 +305,10 @@ func (c *StoreReconciler) ReconcileAll(ctx context.Context) (stats prebuilds.Rec logger.Debug(ctx, "starting reconciliation") - err = c.WithReconciliationLock(ctx, logger, func(ctx context.Context, _ database.Store) error { + err = c.WithReconciliationLock(ctx, logger, func(ctx context.Context, db database.Store) error { // Check if prebuilds reconciliation is paused - settingsJSON, err := c.store.GetPrebuildsSettings(ctx) + // Use db (lock tx) for read-only operations + settingsJSON, err := db.GetPrebuildsSettings(ctx) if err != nil { return xerrors.Errorf("get prebuilds settings: %w", err) } @@ -330,13 +329,16 @@ func (c *StoreReconciler) ReconcileAll(ctx context.Context) (stats prebuilds.Rec return nil } + // MembershipReconciler performs write operations, therefore it needs to use c.store + // directly, since the lock transaction db is read-only. membershipReconciler := NewStoreMembershipReconciler(c.store, c.clock, logger) err = membershipReconciler.ReconcileAll(ctx, database.PrebuildsSystemUserID, PrebuiltWorkspacesGroupName) if err != nil { return xerrors.Errorf("reconcile prebuild membership: %w", err) } - snapshot, err := c.SnapshotState(ctx, c.store) + // Use db (lock tx) for read-only operations + snapshot, err := c.SnapshotState(ctx, db) if err != nil { return xerrors.Errorf("determine current snapshot: %w", err) } @@ -437,6 +439,8 @@ func (c *StoreReconciler) SnapshotState(ctx context.Context, store database.Stor var state prebuilds.GlobalSnapshot + // If called with a store that is already in a transaction, + // InTx will reuse that transaction rather than creating a new one. err := store.InTx(func(db database.Store) error { // TODO: implement template-specific reconciliations later presetsWithPrebuilds, err := db.GetTemplatePresetsWithPrebuilds(ctx, uuid.NullUUID{})