Improvements & tests

Signed-off-by: Danny Kopping <dannykopping@gmail.com> # Conflicts: # coderd/prebuilds/api.go # coderd/prebuilds/noop.go
coder · dannykopping · Apr 25, 2025 · Apr 21, 2025 · Apr 23, 2025 · Apr 23, 2025
commit 9ea19df31f84e3a3390d1d38d77f6ad98473bb1b
diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden
@@ -693,10 +693,11 @@ workspace_prebuilds:
   # How often to reconcile workspace prebuilds state.
   # (default: 15s, type: duration)
   reconciliation_interval: 15s
-  # Interval to increase reconciliation backoff by when unrecoverable errors occur.
+  # Interval to increase reconciliation backoff by when prebuilds fail, after which
+  # a retry attempt is made.
   # (default: 15s, type: duration)
   reconciliation_backoff_interval: 15s
-  # Interval to look back to determine number of failed builds, which influences
+  # Interval to look back to determine number of failed prebuilds, which influences
   # backoff.
   # (default: 1h0m0s, type: duration)
   reconciliation_backoff_lookback_period: 1h0m0s
diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -597,6 +597,7 @@ func New(options *Options) *API {
 	api.AppearanceFetcher.Store(&f)
 	api.PortSharer.Store(&portsharing.DefaultPortSharer)
 	api.PrebuildsClaimer.Store(&prebuilds.DefaultClaimer)
+	api.PrebuildsReconciler.Store(&prebuilds.DefaultReconciler)
 	buildInfo := codersdk.BuildInfoResponse{
 		ExternalURL:           buildinfo.ExternalURL(),
 		Version:               buildinfo.Version(),
@@ -1568,10 +1569,11 @@ type API struct {
 	DERPMapper atomic.Pointer[func(derpMap *tailcfg.DERPMap) *tailcfg.DERPMap]
 	// AccessControlStore is a pointer to an atomic pointer since it is
 	// passed to dbauthz.
-	AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore]
-	PortSharer         atomic.Pointer[portsharing.PortSharer]
-	FileCache          files.Cache
-	PrebuildsClaimer   atomic.Pointer[prebuilds.Claimer]
+	AccessControlStore  *atomic.Pointer[dbauthz.AccessControlStore]
+	PortSharer          atomic.Pointer[portsharing.PortSharer]
+	FileCache           files.Cache
+	PrebuildsClaimer    atomic.Pointer[prebuilds.Claimer]
+	PrebuildsReconciler atomic.Pointer[prebuilds.ReconciliationOrchestrator]
 
 	UpdatesProvider tailnet.WorkspaceUpdatesProvider
 
@@ -1659,6 +1661,13 @@ func (api *API) Close() error {
 	_ = api.AppSigningKeyCache.Close()
 	_ = api.AppEncryptionKeyCache.Close()
 	_ = api.UpdatesProvider.Close()
+
+	if current := api.PrebuildsReconciler.Load(); current != nil {
+		ctx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop"))
+		defer giveUp()
+		(*current).Stop(ctx, nil)
+	}
+
 	return nil
 }
 

diff --git a/coderd/prebuilds/noop.go b/coderd/prebuilds/noop.go
@@ -10,41 +10,28 @@ import (
 
 type NoopReconciler struct{}
 
-func NewNoopReconciler() *NoopReconciler {
-	return &NoopReconciler{}
-}
-
-func (NoopReconciler) RunLoop(context.Context) {}
-
-func (NoopReconciler) Stop(context.Context, error) {}
-
-func (NoopReconciler) ReconcileAll(context.Context) error {
-	return nil
-}
-
+func (NoopReconciler) RunLoop(context.Context)            {}
+func (NoopReconciler) Stop(context.Context, error)        {}
+func (NoopReconciler) ReconcileAll(context.Context) error { return nil }
 func (NoopReconciler) SnapshotState(context.Context, database.Store) (*GlobalSnapshot, error) {
 	return &GlobalSnapshot{}, nil
 }
-
-func (NoopReconciler) ReconcilePreset(context.Context, PresetSnapshot) error {
-	return nil
-}
-
+func (NoopReconciler) ReconcilePreset(context.Context, PresetSnapshot) error { return nil }
 func (NoopReconciler) CalculateActions(context.Context, PresetSnapshot) (*ReconciliationActions, error) {
 	return &ReconciliationActions{}, nil
 }
 
-var _ ReconciliationOrchestrator = NoopReconciler{}
+var DefaultReconciler ReconciliationOrchestrator = NoopReconciler{}
 
-type AGPLPrebuildClaimer struct{}
+type NoopClaimer struct{}
 
-func (AGPLPrebuildClaimer) Claim(context.Context, uuid.UUID, string, uuid.UUID) (*uuid.UUID, error) {
+func (NoopClaimer) Claim(context.Context, uuid.UUID, string, uuid.UUID) (*uuid.UUID, error) {
 	// Not entitled to claim prebuilds in AGPL version.
 	return nil, ErrNoClaimablePrebuiltWorkspaces
 }
 
-func (AGPLPrebuildClaimer) Initiator() uuid.UUID {
+func (NoopClaimer) Initiator() uuid.UUID {
 	return uuid.Nil
 }
 
-var DefaultClaimer Claimer = AGPLPrebuildClaimer{}
+var DefaultClaimer Claimer = NoopClaimer{}
diff --git a/codersdk/deployment.go b/codersdk/deployment.go
@@ -396,8 +396,8 @@ type DeploymentValues struct {
 	TermsOfServiceURL               serpent.String                       `json:"terms_of_service_url,omitempty" typescript:",notnull"`
 	Notifications                   NotificationsConfig                  `json:"notifications,omitempty" typescript:",notnull"`
 	AdditionalCSPPolicy             serpent.StringArray                  `json:"additional_csp_policy,omitempty" typescript:",notnull"`
-	Prebuilds                       PrebuildsConfig                      `json:"workspace_prebuilds,omitempty" typescript:",notnull"`
 	WorkspaceHostnameSuffix         serpent.String                       `json:"workspace_hostname_suffix,omitempty" typescript:",notnull"`
+	Prebuilds                       PrebuildsConfig                      `json:"workspace_prebuilds,omitempty" typescript:",notnull"`
 
 	Config      serpent.YAMLConfigPath `json:"config,omitempty" typescript:",notnull"`
 	WriteConfig serpent.Bool           `json:"write_config,omitempty" typescript:",notnull"`
@@ -3038,6 +3038,9 @@ Write out the current server config as YAML to stdout.`,
 			Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"),
 			Hidden:      true, // Hidden because most operators should not need to modify this.
 		},
+		// Push notifications.
+
+		// Workspace Prebuilds Options
 		{
 			Name:        "Reconciliation Interval",
 			Description: "How often to reconcile workspace prebuilds state.",
@@ -3051,7 +3054,7 @@ Write out the current server config as YAML to stdout.`,
 		},
 		{
 			Name:        "Reconciliation Backoff Interval",
-			Description: "Interval to increase reconciliation backoff by when unrecoverable errors occur.",
+			Description: "Interval to increase reconciliation backoff by when prebuilds fail, after which a retry attempt is made.",
 			Flag:        "workspace-prebuilds-reconciliation-backoff-interval",
 			Env:         "CODER_WORKSPACE_PREBUILDS_RECONCILIATION_BACKOFF_INTERVAL",
 			Value:       &c.Prebuilds.ReconciliationBackoffInterval,
@@ -3063,7 +3066,7 @@ Write out the current server config as YAML to stdout.`,
 		},
 		{
 			Name:        "Reconciliation Backoff Lookback Period",
-			Description: "Interval to look back to determine number of failed builds, which influences backoff.",
+			Description: "Interval to look back to determine number of failed prebuilds, which influences backoff.",
 			Flag:        "workspace-prebuilds-reconciliation-backoff-lookback-period",
 			Env:         "CODER_WORKSPACE_PREBUILDS_RECONCILIATION_BACKOFF_LOOKBACK_PERIOD",
 			Value:       &c.Prebuilds.ReconciliationBackoffLookback,
@@ -3073,7 +3076,6 @@ Write out the current server config as YAML to stdout.`,
 			Annotations: serpent.Annotations{}.Mark(annotationFormatDuration, "true"),
 			Hidden:      true,
 		},
-		// Push notifications.
 	}
 
 	return opts
@@ -3298,9 +3300,9 @@ const (
 	ExperimentAutoFillParameters Experiment = "auto-fill-parameters" // This should not be taken out of experiments until we have redesigned the feature.
 	ExperimentNotifications      Experiment = "notifications"        // Sends notifications via SMTP and webhooks following certain events.
 	ExperimentWorkspaceUsage     Experiment = "workspace-usage"      // Enables the new workspace usage tracking.
-	ExperimentWorkspacePrebuilds Experiment = "workspace-prebuilds"  // Enables the new workspace prebuilds feature.
 	ExperimentWebPush            Experiment = "web-push"             // Enables web push notifications through the browser.
 	ExperimentDynamicParameters  Experiment = "dynamic-parameters"   // Enables dynamic parameters when creating a workspace.
+	ExperimentWorkspacePrebuilds Experiment = "workspace-prebuilds"  // Enables the new workspace prebuilds feature.
 )
 
 // ExperimentsSafe should include all experiments that are safe for

diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md
diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go
@@ -632,8 +632,6 @@ type API struct {
 
 	licenseMetricsCollector *license.MetricsCollector
 	tailnetService          *tailnet.ClientService
-
-	PrebuildsReconciler agplprebuilds.ReconciliationOrchestrator
 }
 
 // writeEntitlementWarningsHeader writes the entitlement warnings to the response header
@@ -665,12 +663,6 @@ func (api *API) Close() error {
 		api.Options.CheckInactiveUsersCancelFunc()
 	}
 
-	if api.PrebuildsReconciler != nil {
-		ctx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop"))
-		defer giveUp()
-		api.PrebuildsReconciler.Stop(ctx, xerrors.New("api closed")) // TODO: determine root cause (requires changes up the stack, though).
-	}
-
 	return api.AGPL.Close()
 }
 
@@ -873,15 +865,15 @@ func (api *API) updateEntitlements(ctx context.Context) error {
 			api.AGPL.PortSharer.Store(&ps)
 		}
 
-		if initial, changed, enabled := featureChanged(codersdk.FeatureWorkspacePrebuilds); shouldUpdate(initial, changed, enabled) || api.PrebuildsReconciler == nil {
+		if initial, changed, enabled := featureChanged(codersdk.FeatureWorkspacePrebuilds); shouldUpdate(initial, changed, enabled) {
 			reconciler, claimer := api.setupPrebuilds(enabled)
-			if api.PrebuildsReconciler != nil {
+			if current := api.AGPL.PrebuildsReconciler.Load(); current != nil {
 				stopCtx, giveUp := context.WithTimeoutCause(context.Background(), time.Second*30, xerrors.New("gave up waiting for reconciler to stop"))
 				defer giveUp()
-				api.PrebuildsReconciler.Stop(stopCtx, xerrors.New("entitlements change"))
+				(*current).Stop(stopCtx, xerrors.New("entitlements change"))
 			}
 
-			api.PrebuildsReconciler = reconciler
+			api.AGPL.PrebuildsReconciler.Store(&reconciler)
 			go reconciler.RunLoop(context.Background())
 
 			api.AGPL.PrebuildsClaimer.Store(&claimer)
@@ -1156,17 +1148,17 @@ func (api *API) Authorize(r *http.Request, action policy.Action, object rbac.Obj
 	return api.AGPL.HTTPAuth.Authorize(r, action, object)
 }
 
-func (api *API) setupPrebuilds(entitled bool) (agplprebuilds.ReconciliationOrchestrator, agplprebuilds.Claimer) {
-	enabled := api.AGPL.Experiments.Enabled(codersdk.ExperimentWorkspacePrebuilds)
-	if !enabled || !entitled {
+// nolint:revive // featureEnabled is a legit control flag.
+func (api *API) setupPrebuilds(featureEnabled bool) (agplprebuilds.ReconciliationOrchestrator, agplprebuilds.Claimer) {
+	experimentEnabled := api.AGPL.Experiments.Enabled(codersdk.ExperimentWorkspacePrebuilds)
+	if !experimentEnabled || !featureEnabled {
 		api.Logger.Debug(context.Background(), "prebuilds not enabled",
-			slog.F("experiment_enabled", enabled), slog.F("entitled", entitled))
+			slog.F("experiment_enabled", experimentEnabled), slog.F("feature_enabled", featureEnabled))
 
-		return agplprebuilds.NewNoopReconciler(), agplprebuilds.DefaultClaimer
+		return agplprebuilds.DefaultReconciler, agplprebuilds.DefaultClaimer
 	}
 
 	reconciler := prebuilds.NewStoreReconciler(api.Database, api.Pubsub, api.DeploymentValues.Prebuilds,
 		api.Logger.Named("prebuilds"), quartz.NewReal())
-
 	return reconciler, prebuilds.EnterpriseClaimer{}
 }