From 2c48c957c4d959beb017633426e1aabcdec1d067 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 2 Jul 2025 12:30:25 +0100 Subject: [PATCH 01/14] chore: add test to ensure correct functionality of GetRunningPrebuiltWorkspaces --- coderd/database/querier_test.go | 142 ++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index f80f68115ad2c..3edee78fd311e 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -5021,3 +5021,145 @@ func requireUsersMatch(t testing.TB, expected []database.User, found []database. t.Helper() require.ElementsMatch(t, expected, database.ConvertUserRows(found), msg) } + +// TestGetRunningPrebuiltWorkspaces ensures the correct behavior of the +// GetRunningPrebuiltWorkspaces query. +func TestGetRunningPrebuiltWorkspaces(t *testing.T) { + t.Parallel() + + if !dbtestutil.WillUsePostgres() { + t.Skip("Test requires PostgreSQL for complex queries") + } + + ctx := testutil.Context(t, testutil.WaitLong) + db, _ := dbtestutil.NewDB(t) + + // Given: a prebuilt workspace with a successful start build and a stop build. + org := dbgen.Organization(t, db, database.Organization{}) + user := dbgen.User(t, db, database.User{}) + template := dbgen.Template(t, db, database.Template{ + CreatedBy: user.ID, + OrganizationID: org.ID, + }) + templateVersion := dbgen.TemplateVersion(t, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + OrganizationID: org.ID, + CreatedBy: user.ID, + }) + preset := dbgen.Preset(t, db, database.InsertPresetParams{ + TemplateVersionID: templateVersion.ID, + DesiredInstances: sql.NullInt32{Int32: 1, Valid: true}, + }) + + // Create a prebuild workspace (owned by system user) + prebuildSystemUser := uuid.MustParse("c42fdf75-3097-471c-8c33-fb52454d81c0") + stoppedPrebuild := dbgen.Workspace(t, db, database.WorkspaceTable{ + OwnerID: prebuildSystemUser, + TemplateID: template.ID, + Name: "test-prebuild", + Deleted: false, + }) + + // Create a successful START build + stoppedPrebuildJob1 := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: database.PrebuildsSystemUserID, + Provisioner: database.ProvisionerTypeEcho, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StartedAt: sql.NullTime{Time: dbtime.Now().Add(-time.Minute), Valid: true}, + CompletedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, + Error: sql.NullString{}, + ErrorCode: sql.NullString{}, + }) + stoppedPrebuiltWorkspaceBuild1 := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: stoppedPrebuild.ID, + TemplateVersionID: templateVersion.ID, + TemplateVersionPresetID: uuid.NullUUID{UUID: preset.ID, Valid: true}, + JobID: stoppedPrebuildJob1.ID, + BuildNumber: 1, + Transition: database.WorkspaceTransitionStart, + InitiatorID: database.PrebuildsSystemUserID, + Reason: database.BuildReasonInitiator, + }) + + // Create a STOP build (making this prebuild "not running") + stoppedPrebuildWorkspaceJob2 := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: database.PrebuildsSystemUserID, + Provisioner: database.ProvisionerTypeEcho, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StartedAt: sql.NullTime{Time: dbtime.Now().Add(-time.Minute), Valid: true}, + CompletedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, + Error: sql.NullString{}, + ErrorCode: sql.NullString{}, + }) + stoppedPrebuiltWorkspaceBuild2 := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: stoppedPrebuild.ID, + TemplateVersionID: templateVersion.ID, + TemplateVersionPresetID: uuid.NullUUID{UUID: preset.ID, Valid: true}, + JobID: stoppedPrebuildWorkspaceJob2.ID, + BuildNumber: 2, + Transition: database.WorkspaceTransitionStop, + InitiatorID: database.PrebuildsSystemUserID, + Reason: database.BuildReasonInitiator, + }) + + // Create a second running prebuild workspace with a successful start build + // and no stop build. + runningPrebuild := dbgen.Workspace(t, db, database.WorkspaceTable{ + OwnerID: prebuildSystemUser, + TemplateID: template.ID, + Name: "test-running-prebuild", + Deleted: false, + }) + // Create a successful START build for the running prebuild workspace + runningPrebuildJob1 := dbgen.ProvisionerJob(t, db, nil, + database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: user.ID, + Provisioner: database.ProvisionerTypeEcho, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StartedAt: sql.NullTime{Time: dbtime.Now().Add(-time.Minute), Valid: true}, + CompletedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, + Error: sql.NullString{}, + ErrorCode: sql.NullString{}, + }) + runningPrebuiltWorkspaceBuild1 := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: runningPrebuild.ID, + TemplateVersionID: templateVersion.ID, + TemplateVersionPresetID: uuid.NullUUID{UUID: preset.ID, Valid: true}, + JobID: runningPrebuildJob1.ID, + BuildNumber: 1, + Transition: database.WorkspaceTransitionStart, + InitiatorID: database.PrebuildsSystemUserID, + Reason: database.BuildReasonInitiator, + }) + + // Create a third running regular workspace (not a prebuild) with a successful + // start build. + _ = dbgen.Workspace(t, db, database.WorkspaceTable{ + OwnerID: user.ID, + TemplateID: template.ID, + Name: "test-running-regular-workspace", + Deleted: false, + }) + + // Given: assert test invariants. + require.Equal(t, database.WorkspaceTransitionStart, stoppedPrebuiltWorkspaceBuild1.Transition) + require.Equal(t, int32(1), stoppedPrebuiltWorkspaceBuild1.BuildNumber) + require.Equal(t, database.ProvisionerJobStatusSucceeded, stoppedPrebuildJob1.JobStatus) + require.Equal(t, database.WorkspaceTransitionStop, stoppedPrebuiltWorkspaceBuild2.Transition) + require.Equal(t, int32(2), stoppedPrebuiltWorkspaceBuild2.BuildNumber) + require.Equal(t, database.ProvisionerJobStatusSucceeded, stoppedPrebuildWorkspaceJob2.JobStatus) + require.Equal(t, database.WorkspaceTransitionStart, runningPrebuiltWorkspaceBuild1.Transition) + require.Equal(t, int32(1), runningPrebuiltWorkspaceBuild1.BuildNumber) + require.Equal(t, database.ProvisionerJobStatusSucceeded, runningPrebuildJob1.JobStatus) + + // When: we query for running prebuild workspaces + runningPrebuilds, err := db.GetRunningPrebuiltWorkspaces(ctx) + require.NoError(t, err) + + // Then: the stopped prebuild workspace should not be returned. + require.Len(t, runningPrebuilds, 1, "expected only one running prebuilt workspace") + require.Equal(t, runningPrebuild.ID, runningPrebuilds[0].ID, "expected the running prebuilt workspace to be returned") +} From 68a430431fd1a22bb6434f1bffd79e28d3e7fc62 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 2 Jul 2025 13:14:31 +0100 Subject: [PATCH 02/14] chore: add test antagonists to enterprise/prebuild/reconcile tests --- enterprise/coderd/prebuilds/reconcile_test.go | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index fce5269214ed1..5ba36912ce5c8 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -370,6 +370,8 @@ func TestPrebuildReconciliation(t *testing.T) { templateVersionID, ) + setupTestDBPrebuildAntagonists(t, db, pubSub, org) + if !templateVersionActive { // Create a new template version and mark it as active // This marks the template version that we care about as inactive @@ -2116,6 +2118,115 @@ func setupTestDBWorkspaceAgent(t *testing.T, db database.Store, workspaceID uuid return agent } +// setupTestDBAntagonists creates test antagonists that should not influence running prebuild workspace tests. +// 1. A stopped prebuilt workspace (STOP then START transitions, owned by +// prebuilds system user). +// 2. A running regular workspace (not owned by the prebuilds system user). +func setupTestDBPrebuildAntagonists(t *testing.T, db database.Store, ps pubsub.Pubsub, org database.Organization) { + t.Helper() + + templateAdmin := dbgen.User(t, db, database.User{RBACRoles: []string{codersdk.RoleTemplateAdmin}}) + _ = dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: org.ID, + UserID: templateAdmin.ID, + }) + member := dbgen.User(t, db, database.User{}) + _ = dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: org.ID, + UserID: member.ID, + }) + tpl := dbgen.Template(t, db, database.Template{ + OrganizationID: org.ID, + CreatedBy: templateAdmin.ID, + }) + tv := dbgen.TemplateVersion(t, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true}, + OrganizationID: org.ID, + CreatedBy: templateAdmin.ID, + }) + + // 1) Stopped prebuilt workspace (owned by prebuilds system user) + stoppedPrebuild := dbgen.Workspace(t, db, database.WorkspaceTable{ + OwnerID: database.PrebuildsSystemUserID, + TemplateID: tpl.ID, + Name: "prebuild-antagonist-stopped", + Deleted: false, + }) + + // STOP build (build number 2, most recent) + stoppedJob2 := dbgen.ProvisionerJob(t, db, ps, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: database.PrebuildsSystemUserID, + Provisioner: database.ProvisionerTypeEcho, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StartedAt: sql.NullTime{Time: dbtime.Now().Add(-30 * time.Second), Valid: true}, + CompletedAt: sql.NullTime{Time: dbtime.Now().Add(-20 * time.Second), Valid: true}, + Error: sql.NullString{}, + ErrorCode: sql.NullString{}, + }) + dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: stoppedPrebuild.ID, + TemplateVersionID: tv.ID, + JobID: stoppedJob2.ID, + BuildNumber: 2, + Transition: database.WorkspaceTransitionStop, + InitiatorID: database.PrebuildsSystemUserID, + Reason: database.BuildReasonInitiator, + // Explicitly not using a preset here. This shouldn't normally be possible, + // but without this the reconciler will try to create a new prebuild for + // this preset, which will affect the tests. + TemplateVersionPresetID: uuid.NullUUID{}, + }) + + // START build (build number 1, older) + stoppedJob1 := dbgen.ProvisionerJob(t, db, ps, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: database.PrebuildsSystemUserID, + Provisioner: database.ProvisionerTypeEcho, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StartedAt: sql.NullTime{Time: dbtime.Now().Add(-60 * time.Second), Valid: true}, + CompletedAt: sql.NullTime{Time: dbtime.Now().Add(-50 * time.Second), Valid: true}, + Error: sql.NullString{}, + ErrorCode: sql.NullString{}, + }) + dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: stoppedPrebuild.ID, + TemplateVersionID: tv.ID, + JobID: stoppedJob1.ID, + BuildNumber: 1, + Transition: database.WorkspaceTransitionStart, + InitiatorID: database.PrebuildsSystemUserID, + Reason: database.BuildReasonInitiator, + }) + + // 2) Running regular workspace (not owned by prebuilds system user) + regularWorkspace := dbgen.Workspace(t, db, database.WorkspaceTable{ + OwnerID: member.ID, + TemplateID: tpl.ID, + Name: "antagonist-regular-workspace", + Deleted: false, + }) + regularJob := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ + OrganizationID: org.ID, + InitiatorID: member.ID, + Provisioner: database.ProvisionerTypeEcho, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StartedAt: sql.NullTime{Time: dbtime.Now().Add(-40 * time.Second), Valid: true}, + CompletedAt: sql.NullTime{Time: dbtime.Now().Add(-30 * time.Second), Valid: true}, + Error: sql.NullString{}, + ErrorCode: sql.NullString{}, + }) + dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: regularWorkspace.ID, + TemplateVersionID: tv.ID, + JobID: regularJob.ID, + BuildNumber: 1, + Transition: database.WorkspaceTransitionStart, + InitiatorID: member.ID, + Reason: database.BuildReasonInitiator, + }) +} + var allTransitions = []database.WorkspaceTransition{ database.WorkspaceTransitionStart, database.WorkspaceTransitionStop, From c95b6d06c2ea078f24e7ff03a791928bff7a45b1 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 2 Jul 2025 13:48:37 +0100 Subject: [PATCH 03/14] chore: alternate optimization for GetRunningPrebuiltWorkspaces --- coderd/database/queries.sql.go | 48 ++++++++++++++++++++------ coderd/database/queries/prebuilds.sql | 49 +++++++++++++++++++++------ 2 files changed, 75 insertions(+), 22 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 580b621b0908a..e3aa4ca3962f2 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6444,18 +6444,44 @@ func (q *sqlQuerier) GetPresetsBackoff(ctx context.Context, lookback time.Time) } const getRunningPrebuiltWorkspaces = `-- name: GetRunningPrebuiltWorkspaces :many +WITH latest_prebuilds AS ( + SELECT + workspaces.id, + workspaces.name, + workspaces.template_id, + workspace_latest_builds.template_version_id, + workspace_latest_builds.template_version_preset_id, + workspace_latest_builds.job_id, + workspaces.created_at + FROM workspace_latest_builds + JOIN workspaces ON workspaces.id = workspace_latest_builds.workspace_id + WHERE workspace_latest_builds.transition = 'start'::workspace_transition + AND workspace_latest_builds.job_status = 'succeeded'::provisioner_job_status + AND workspaces.owner_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::UUID + AND NOT workspaces.deleted +), +ready_agents AS ( + SELECT + latest_prebuilds.job_id, + BOOL_AND(workspace_agents.lifecycle_state = 'ready'::workspace_agent_lifecycle_state)::boolean AS ready + FROM latest_prebuilds + JOIN workspace_resources ON workspace_resources.job_id = latest_prebuilds.job_id + JOIN workspace_agents ON workspace_agents.resource_id = workspace_resources.id + WHERE workspace_agents.deleted = false + AND workspace_agents.parent_id IS NULL + GROUP BY latest_prebuilds.job_id +) SELECT - p.id, - p.name, - p.template_id, - b.template_version_id, - p.current_preset_id AS current_preset_id, - p.ready, - p.created_at -FROM workspace_prebuilds p - INNER JOIN workspace_latest_builds b ON b.workspace_id = p.id -WHERE (b.transition = 'start'::workspace_transition - AND b.job_status = 'succeeded'::provisioner_job_status) + latest_prebuilds.id, + latest_prebuilds.name, + latest_prebuilds.template_id, + latest_prebuilds.template_version_id, + -- TODO(cian): this can be null, which differs from prebuilt_workspaces view. + latest_prebuilds.template_version_preset_id AS current_preset_id, + COALESCE(ready_agents.ready, false)::boolean AS ready, + latest_prebuilds.created_at +FROM latest_prebuilds +LEFT JOIN ready_agents ON ready_agents.job_id = latest_prebuilds.job_id ` type GetRunningPrebuiltWorkspacesRow struct { diff --git a/coderd/database/queries/prebuilds.sql b/coderd/database/queries/prebuilds.sql index 2fc9f3f4a67f6..2cb4e45898ab9 100644 --- a/coderd/database/queries/prebuilds.sql +++ b/coderd/database/queries/prebuilds.sql @@ -49,18 +49,45 @@ WHERE tvp.desired_instances IS NOT NULL -- Consider only presets that have a pre AND (t.id = sqlc.narg('template_id')::uuid OR sqlc.narg('template_id') IS NULL); -- name: GetRunningPrebuiltWorkspaces :many +WITH latest_prebuilds AS ( + SELECT + workspaces.id, + workspaces.name, + workspaces.template_id, + workspace_latest_builds.template_version_id, + workspace_latest_builds.template_version_preset_id, + workspace_latest_builds.job_id, + workspaces.created_at + FROM workspace_latest_builds + JOIN workspaces ON workspaces.id = workspace_latest_builds.workspace_id + WHERE workspace_latest_builds.transition = 'start'::workspace_transition + AND workspace_latest_builds.job_status = 'succeeded'::provisioner_job_status + AND workspaces.owner_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::UUID + AND NOT workspaces.deleted +), +ready_agents AS ( + SELECT + latest_prebuilds.job_id, + BOOL_AND(workspace_agents.lifecycle_state = 'ready'::workspace_agent_lifecycle_state)::boolean AS ready + FROM latest_prebuilds + JOIN workspace_resources ON workspace_resources.job_id = latest_prebuilds.job_id + JOIN workspace_agents ON workspace_agents.resource_id = workspace_resources.id + WHERE workspace_agents.deleted = false + AND workspace_agents.parent_id IS NULL + GROUP BY latest_prebuilds.job_id +) SELECT - p.id, - p.name, - p.template_id, - b.template_version_id, - p.current_preset_id AS current_preset_id, - p.ready, - p.created_at -FROM workspace_prebuilds p - INNER JOIN workspace_latest_builds b ON b.workspace_id = p.id -WHERE (b.transition = 'start'::workspace_transition - AND b.job_status = 'succeeded'::provisioner_job_status); + latest_prebuilds.id, + latest_prebuilds.name, + latest_prebuilds.template_id, + latest_prebuilds.template_version_id, + -- TODO(cian): this can be null, which differs from prebuilt_workspaces view. + latest_prebuilds.template_version_preset_id AS current_preset_id, + COALESCE(ready_agents.ready, false)::boolean AS ready, + latest_prebuilds.created_at +FROM latest_prebuilds +LEFT JOIN ready_agents ON ready_agents.job_id = latest_prebuilds.job_id +; -- name: CountInProgressPrebuilds :many -- CountInProgressPrebuilds returns the number of in-progress prebuilds, grouped by preset ID and transition. From 6b4d6dc251c7682213dce9fb590f1db2997bd0fb Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 2 Jul 2025 14:54:37 +0100 Subject: [PATCH 04/14] chore: get latest preset id --- coderd/database/queries.sql.go | 15 ++++++++++++--- coderd/database/queries/prebuilds.sql | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index e3aa4ca3962f2..79405e9ccf120 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6450,7 +6450,6 @@ WITH latest_prebuilds AS ( workspaces.name, workspaces.template_id, workspace_latest_builds.template_version_id, - workspace_latest_builds.template_version_preset_id, workspace_latest_builds.job_id, workspaces.created_at FROM workspace_latest_builds @@ -6460,6 +6459,16 @@ WITH latest_prebuilds AS ( AND workspaces.owner_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::UUID AND NOT workspaces.deleted ), +workspace_latest_presets AS ( + SELECT DISTINCT ON (latest_prebuilds.id) + latest_prebuilds.id AS workspace_id, + workspace_builds.template_version_preset_id AS current_preset_id + FROM latest_prebuilds + JOIN workspace_builds ON workspace_builds.workspace_id = latest_prebuilds.id + WHERE workspace_builds.transition = 'start'::workspace_transition + AND workspace_builds.template_version_preset_id IS NOT NULL + ORDER BY latest_prebuilds.id, workspace_builds.build_number DESC +), ready_agents AS ( SELECT latest_prebuilds.job_id, @@ -6476,12 +6485,12 @@ SELECT latest_prebuilds.name, latest_prebuilds.template_id, latest_prebuilds.template_version_id, - -- TODO(cian): this can be null, which differs from prebuilt_workspaces view. - latest_prebuilds.template_version_preset_id AS current_preset_id, + workspace_latest_presets.current_preset_id, COALESCE(ready_agents.ready, false)::boolean AS ready, latest_prebuilds.created_at FROM latest_prebuilds LEFT JOIN ready_agents ON ready_agents.job_id = latest_prebuilds.job_id +LEFT JOIN workspace_latest_presets ON workspace_latest_presets.workspace_id = latest_prebuilds.id ` type GetRunningPrebuiltWorkspacesRow struct { diff --git a/coderd/database/queries/prebuilds.sql b/coderd/database/queries/prebuilds.sql index 2cb4e45898ab9..4c5473a57b628 100644 --- a/coderd/database/queries/prebuilds.sql +++ b/coderd/database/queries/prebuilds.sql @@ -55,7 +55,6 @@ WITH latest_prebuilds AS ( workspaces.name, workspaces.template_id, workspace_latest_builds.template_version_id, - workspace_latest_builds.template_version_preset_id, workspace_latest_builds.job_id, workspaces.created_at FROM workspace_latest_builds @@ -65,6 +64,16 @@ WITH latest_prebuilds AS ( AND workspaces.owner_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::UUID AND NOT workspaces.deleted ), +workspace_latest_presets AS ( + SELECT DISTINCT ON (latest_prebuilds.id) + latest_prebuilds.id AS workspace_id, + workspace_builds.template_version_preset_id AS current_preset_id + FROM latest_prebuilds + JOIN workspace_builds ON workspace_builds.workspace_id = latest_prebuilds.id + WHERE workspace_builds.transition = 'start'::workspace_transition + AND workspace_builds.template_version_preset_id IS NOT NULL + ORDER BY latest_prebuilds.id, workspace_builds.build_number DESC +), ready_agents AS ( SELECT latest_prebuilds.job_id, @@ -81,12 +90,12 @@ SELECT latest_prebuilds.name, latest_prebuilds.template_id, latest_prebuilds.template_version_id, - -- TODO(cian): this can be null, which differs from prebuilt_workspaces view. - latest_prebuilds.template_version_preset_id AS current_preset_id, + workspace_latest_presets.current_preset_id, COALESCE(ready_agents.ready, false)::boolean AS ready, latest_prebuilds.created_at FROM latest_prebuilds LEFT JOIN ready_agents ON ready_agents.job_id = latest_prebuilds.job_id +LEFT JOIN workspace_latest_presets ON workspace_latest_presets.workspace_id = latest_prebuilds.id ; -- name: CountInProgressPrebuilds :many From 53c3ba5a03a9e6b859d9c4039672f05d65db318a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 3 Jul 2025 13:59:14 +0100 Subject: [PATCH 05/14] run both queries and diff results --- coderd/database/dbauthz/dbauthz.go | 8 + coderd/database/dbmem/dbmem.go | 4 + coderd/database/dbmetrics/querymetrics.go | 7 + coderd/database/dbmock/dbmock.go | 30 ++++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 68 +++++++- coderd/database/queries/prebuilds.sql | 19 ++- enterprise/coderd/prebuilds/reconcile.go | 38 +++++ enterprise/coderd/prebuilds/reconcile_test.go | 148 ++++++++++++++++++ 9 files changed, 317 insertions(+), 6 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 65630849084b1..6daa7119dcffe 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2560,6 +2560,14 @@ func (q *querier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]database. return q.db.GetRunningPrebuiltWorkspaces(ctx) } +func (q *querier) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesOptimizedRow, error) { + // This query returns only prebuilt workspaces, but we decided to require permissions for all workspaces. + if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceWorkspace.All()); err != nil { + return nil, err + } + return q.db.GetRunningPrebuiltWorkspacesOptimized(ctx) +} + func (q *querier) GetRuntimeConfig(ctx context.Context, key string) (string, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { return "", err diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 1c65abd29eb7f..bd0d87dd7fd18 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -5101,6 +5101,10 @@ func (q *FakeQuerier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]datab return nil, ErrUnimplemented } +func (q *FakeQuerier) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesOptimizedRow, error) { + panic("not implemented") +} + func (q *FakeQuerier) GetRuntimeConfig(_ context.Context, key string) (string, error) { q.mutex.Lock() defer q.mutex.Unlock() diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 6c633fe8c5c2f..7e2e0ba1f993d 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -1313,6 +1313,13 @@ func (m queryMetricsStore) GetRunningPrebuiltWorkspaces(ctx context.Context) ([] return r0, r1 } +func (m queryMetricsStore) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesOptimizedRow, error) { + start := time.Now() + r0, r1 := m.s.GetRunningPrebuiltWorkspacesOptimized(ctx) + m.queryLatencies.WithLabelValues("GetRunningPrebuiltWorkspacesOptimized").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m queryMetricsStore) GetRuntimeConfig(ctx context.Context, key string) (string, error) { start := time.Now() r0, r1 := m.s.GetRuntimeConfig(ctx, key) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 368cb021ab7ca..7b5a64f139b4f 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -2733,6 +2733,36 @@ func (mr *MockStoreMockRecorder) GetRunningPrebuiltWorkspaces(ctx any) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRunningPrebuiltWorkspaces", reflect.TypeOf((*MockStore)(nil).GetRunningPrebuiltWorkspaces), ctx) } +// GetRunningPrebuiltWorkspacesOptimized mocks base method. +func (m *MockStore) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesOptimizedRow, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRunningPrebuiltWorkspacesOptimized", ctx) + ret0, _ := ret[0].([]database.GetRunningPrebuiltWorkspacesOptimizedRow) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetRunningPrebuiltWorkspacesOptimized indicates an expected call of GetRunningPrebuiltWorkspacesOptimized. +func (mr *MockStoreMockRecorder) GetRunningPrebuiltWorkspacesOptimized(ctx any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRunningPrebuiltWorkspacesOptimized", reflect.TypeOf((*MockStore)(nil).GetRunningPrebuiltWorkspacesOptimized), ctx) +} + +// GetRunningPrebuiltWorkspacesWithComparison mocks base method. +func (m *MockStore) GetRunningPrebuiltWorkspacesWithComparison(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesRow, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRunningPrebuiltWorkspacesWithComparison", ctx) + ret0, _ := ret[0].([]database.GetRunningPrebuiltWorkspacesRow) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetRunningPrebuiltWorkspacesWithComparison indicates an expected call of GetRunningPrebuiltWorkspacesWithComparison. +func (mr *MockStoreMockRecorder) GetRunningPrebuiltWorkspacesWithComparison(ctx any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRunningPrebuiltWorkspacesWithComparison", reflect.TypeOf((*MockStore)(nil).GetRunningPrebuiltWorkspacesWithComparison), ctx) +} + // GetRuntimeConfig mocks base method. func (m *MockStore) GetRuntimeConfig(ctx context.Context, key string) (string, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 4b69e192738f4..f26473e8c419d 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -293,6 +293,7 @@ type sqlcQuerier interface { GetReplicaByID(ctx context.Context, id uuid.UUID) (Replica, error) GetReplicasUpdatedAfter(ctx context.Context, updatedAt time.Time) ([]Replica, error) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]GetRunningPrebuiltWorkspacesRow, error) + GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]GetRunningPrebuiltWorkspacesOptimizedRow, error) GetRuntimeConfig(ctx context.Context, key string) (string, error) GetTailnetAgents(ctx context.Context, id uuid.UUID) ([]TailnetAgent, error) GetTailnetClientsForAgent(ctx context.Context, agentID uuid.UUID) ([]TailnetClient, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 79405e9ccf120..7d3ce70b46fe5 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6444,6 +6444,63 @@ func (q *sqlQuerier) GetPresetsBackoff(ctx context.Context, lookback time.Time) } const getRunningPrebuiltWorkspaces = `-- name: GetRunningPrebuiltWorkspaces :many +SELECT + p.id, + p.name, + p.template_id, + b.template_version_id, + p.current_preset_id AS current_preset_id, + p.ready, + p.created_at +FROM workspace_prebuilds p + INNER JOIN workspace_latest_builds b ON b.workspace_id = p.id +WHERE (b.transition = 'start'::workspace_transition + AND b.job_status = 'succeeded'::provisioner_job_status) +ORDER BY p.id +` + +type GetRunningPrebuiltWorkspacesRow struct { + ID uuid.UUID `db:"id" json:"id"` + Name string `db:"name" json:"name"` + TemplateID uuid.UUID `db:"template_id" json:"template_id"` + TemplateVersionID uuid.UUID `db:"template_version_id" json:"template_version_id"` + CurrentPresetID uuid.NullUUID `db:"current_preset_id" json:"current_preset_id"` + Ready bool `db:"ready" json:"ready"` + CreatedAt time.Time `db:"created_at" json:"created_at"` +} + +func (q *sqlQuerier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]GetRunningPrebuiltWorkspacesRow, error) { + rows, err := q.db.QueryContext(ctx, getRunningPrebuiltWorkspaces) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetRunningPrebuiltWorkspacesRow + for rows.Next() { + var i GetRunningPrebuiltWorkspacesRow + if err := rows.Scan( + &i.ID, + &i.Name, + &i.TemplateID, + &i.TemplateVersionID, + &i.CurrentPresetID, + &i.Ready, + &i.CreatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const getRunningPrebuiltWorkspacesOptimized = `-- name: GetRunningPrebuiltWorkspacesOptimized :many WITH latest_prebuilds AS ( SELECT workspaces.id, @@ -6491,9 +6548,10 @@ SELECT FROM latest_prebuilds LEFT JOIN ready_agents ON ready_agents.job_id = latest_prebuilds.job_id LEFT JOIN workspace_latest_presets ON workspace_latest_presets.workspace_id = latest_prebuilds.id +ORDER BY latest_prebuilds.id ` -type GetRunningPrebuiltWorkspacesRow struct { +type GetRunningPrebuiltWorkspacesOptimizedRow struct { ID uuid.UUID `db:"id" json:"id"` Name string `db:"name" json:"name"` TemplateID uuid.UUID `db:"template_id" json:"template_id"` @@ -6503,15 +6561,15 @@ type GetRunningPrebuiltWorkspacesRow struct { CreatedAt time.Time `db:"created_at" json:"created_at"` } -func (q *sqlQuerier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]GetRunningPrebuiltWorkspacesRow, error) { - rows, err := q.db.QueryContext(ctx, getRunningPrebuiltWorkspaces) +func (q *sqlQuerier) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]GetRunningPrebuiltWorkspacesOptimizedRow, error) { + rows, err := q.db.QueryContext(ctx, getRunningPrebuiltWorkspacesOptimized) if err != nil { return nil, err } defer rows.Close() - var items []GetRunningPrebuiltWorkspacesRow + var items []GetRunningPrebuiltWorkspacesOptimizedRow for rows.Next() { - var i GetRunningPrebuiltWorkspacesRow + var i GetRunningPrebuiltWorkspacesOptimizedRow if err := rows.Scan( &i.ID, &i.Name, diff --git a/coderd/database/queries/prebuilds.sql b/coderd/database/queries/prebuilds.sql index 4c5473a57b628..6d287205c11cc 100644 --- a/coderd/database/queries/prebuilds.sql +++ b/coderd/database/queries/prebuilds.sql @@ -48,7 +48,7 @@ WHERE tvp.desired_instances IS NOT NULL -- Consider only presets that have a pre -- AND NOT t.deleted -- We don't exclude deleted templates because there's no constraint in the DB preventing a soft deletion on a template while workspaces are running. AND (t.id = sqlc.narg('template_id')::uuid OR sqlc.narg('template_id') IS NULL); --- name: GetRunningPrebuiltWorkspaces :many +-- name: GetRunningPrebuiltWorkspacesOptimized :many WITH latest_prebuilds AS ( SELECT workspaces.id, @@ -96,6 +96,23 @@ SELECT FROM latest_prebuilds LEFT JOIN ready_agents ON ready_agents.job_id = latest_prebuilds.job_id LEFT JOIN workspace_latest_presets ON workspace_latest_presets.workspace_id = latest_prebuilds.id +ORDER BY latest_prebuilds.id +; + +-- name: GetRunningPrebuiltWorkspaces :many +SELECT + p.id, + p.name, + p.template_id, + b.template_version_id, + p.current_preset_id AS current_preset_id, + p.ready, + p.created_at +FROM workspace_prebuilds p + INNER JOIN workspace_latest_builds b ON b.workspace_id = p.id +WHERE (b.transition = 'start'::workspace_transition + AND b.job_status = 'succeeded'::provisioner_job_status) +ORDER BY p.id; ; -- name: CountInProgressPrebuilds :many diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 44e0e82c8881a..3beb27acec5fd 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -12,6 +12,8 @@ import ( "sync/atomic" "time" + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/go-multierror" "github.com/prometheus/client_golang/prometheus" @@ -398,11 +400,21 @@ func (c *StoreReconciler) SnapshotState(ctx context.Context, store database.Stor return xerrors.Errorf("failed to get preset prebuild schedules: %w", err) } + // Get results from both original and optimized queries for comparison allRunningPrebuilds, err := db.GetRunningPrebuiltWorkspaces(ctx) if err != nil { return xerrors.Errorf("failed to get running prebuilds: %w", err) } + // Compare with optimized query to ensure behavioral correctness + optimized, err := db.GetRunningPrebuiltWorkspacesOptimized(ctx) + if err != nil { + // Log the error but continue with original results + c.logger.Error(ctx, "optimized GetRunningPrebuiltWorkspacesOptimized failed", slog.Error(err)) + } else { + CompareGetRunningPrebuiltWorkspacesResults(ctx, c.logger, allRunningPrebuilds, optimized) + } + allPrebuildsInProgress, err := db.CountInProgressPrebuilds(ctx) if err != nil { return xerrors.Errorf("failed to get prebuilds in progress: %w", err) @@ -922,3 +934,29 @@ func SetPrebuildsReconciliationPaused(ctx context.Context, db database.Store, pa } return db.UpsertPrebuildsSettings(ctx, string(settingsJSON)) } + +// CompareGetRunningPrebuiltWorkspacesResults compares the original and optimized +// query results and logs any differences found. This function can be easily +// removed once we're confident the optimized query works correctly. +func CompareGetRunningPrebuiltWorkspacesResults( + ctx context.Context, + logger slog.Logger, + original []database.GetRunningPrebuiltWorkspacesRow, + optimized []database.GetRunningPrebuiltWorkspacesOptimizedRow, +) { + // Convert optimized results to the same type as original for comparison + var optimizedConverted []database.GetRunningPrebuiltWorkspacesRow + if original != nil { + optimizedConverted := make([]database.GetRunningPrebuiltWorkspacesRow, len(optimized)) + for i, row := range optimized { + optimizedConverted[i] = database.GetRunningPrebuiltWorkspacesRow(row) + } + } + + // Compare the results and log an error if they differ. + // NOTE: explicitly not sorting here as both query results are ordered by ID. + if diff := cmp.Diff(original, optimizedConverted); diff != "" { + logger.Error(ctx, "results differ for GetRunningPrebuiltWorkspacesOptimized", + slog.F("diff", diff)) + } +} diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index 5ba36912ce5c8..7ead88a786b76 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -2331,3 +2331,151 @@ func TestReconciliationRespectsPauseSetting(t *testing.T) { require.NoError(t, err) require.Len(t, workspaces, 2, "should have recreated 2 prebuilds after resuming") } + +func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + // Helper to create test data + createWorkspaceRow := func(id string, name string, ready bool) database.GetRunningPrebuiltWorkspacesRow { + uid := uuid.MustParse(id) + return database.GetRunningPrebuiltWorkspacesRow{ + ID: uid, + Name: name, + TemplateID: uuid.New(), + TemplateVersionID: uuid.New(), + CurrentPresetID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, + Ready: ready, + CreatedAt: time.Now(), + } + } + + createOptimizedRow := func(row database.GetRunningPrebuiltWorkspacesRow) database.GetRunningPrebuiltWorkspacesOptimizedRow { + return database.GetRunningPrebuiltWorkspacesOptimizedRow(row) + } + + t.Run("identical results - no logging", func(t *testing.T) { + t.Parallel() + + spy := &logSpy{} + logger := slog.Make(spy) + + original := []database.GetRunningPrebuiltWorkspacesRow{ + createWorkspaceRow("550e8400-e29b-41d4-a716-446655440000", "workspace1", true), + createWorkspaceRow("550e8400-e29b-41d4-a716-446655440001", "workspace2", false), + } + + optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{ + createOptimizedRow(original[0]), + createOptimizedRow(original[1]), + } + + prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) + + // Should not log any errors when results are identical + require.Empty(t, spy.entries) + }) + + t.Run("count mismatch - logs error", func(t *testing.T) { + t.Parallel() + + spy := &logSpy{} + logger := slog.Make(spy) + + original := []database.GetRunningPrebuiltWorkspacesRow{ + createWorkspaceRow("550e8400-e29b-41d4-a716-446655440000", "workspace1", true), + } + + optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{ + createOptimizedRow(original[0]), + createOptimizedRow(createWorkspaceRow("550e8400-e29b-41d4-a716-446655440001", "workspace2", false)), + } + + prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) + + // Should log exactly one error for count mismatch + require.Len(t, spy.entries, 1) + assert.Equal(t, slog.LevelError, spy.entries[0].Level) + assert.Equal(t, "result count mismatch for GetRunningPrebuiltWorkspacesOptimized", spy.entries[0].Message) + assert.Equal(t, 1, spy.getField(0, "original_count")) + assert.Equal(t, 2, spy.getField(0, "optimized_count")) + }) + + t.Run("field differences - logs errors", func(t *testing.T) { + t.Parallel() + + spy := &logSpy{} + logger := slog.Make(spy) + + workspace1 := createWorkspaceRow("550e8400-e29b-41d4-a716-446655440000", "workspace1", true) + workspace2 := createWorkspaceRow("550e8400-e29b-41d4-a716-446655440001", "workspace2", false) + + original := []database.GetRunningPrebuiltWorkspacesRow{workspace1, workspace2} + + // Create optimized with different values + optimized1 := createOptimizedRow(workspace1) + optimized1.Name = "different-name" // Different name + optimized1.Ready = false // Different ready status + + optimized2 := createOptimizedRow(workspace2) + optimized2.CurrentPresetID = uuid.NullUUID{Valid: false} // Different preset ID (NULL) + + optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{optimized1, optimized2} + + prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) + + // Should log exactly one error with a cmp.Diff output + require.Len(t, spy.entries, 1) + assert.Equal(t, slog.LevelError, spy.entries[0].Level) + assert.Equal(t, "GetRunningPrebuiltWorkspacesOptimized results differ from original", spy.entries[0].Message) + + diff := spy.getField(0, "diff") + require.NotNil(t, diff) + diffStr := diff.(string) + + // The diff should contain information about the differences we introduced + assert.Contains(t, diffStr, "different-name") // Changed name + assert.Contains(t, diffStr, "workspace1") // Original name + assert.Contains(t, diffStr, "Ready") // Changed ready status + assert.Contains(t, diffStr, "CurrentPresetID") // Changed preset ID + }) + + t.Run("empty results - no logging", func(t *testing.T) { + t.Parallel() + + spy := &logSpy{} + logger := slog.Make(spy) + + original := []database.GetRunningPrebuiltWorkspacesRow{} + optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{} + + prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) + + // Should not log any errors when both results are empty + require.Empty(t, spy.entries) + }) +} + +// logSpy captures log entries for testing +type logSpy struct { + entries []slog.SinkEntry +} + +func (s *logSpy) LogEntry(_ context.Context, e slog.SinkEntry) { + s.entries = append(s.entries, e) +} + +func (*logSpy) Sync() {} + +func (s *logSpy) getField(entryIndex int, fieldName string) interface{} { + if entryIndex >= len(s.entries) { + return nil + } + for _, field := range s.entries[entryIndex].Fields { + if field.Name == fieldName { + return field.Value + } + } + return nil +} From b2372512f90d093eed1cc3d1d1151d2aa53ae8c4 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 3 Jul 2025 14:31:07 +0100 Subject: [PATCH 06/14] fixup! run both queries and diff results --- enterprise/coderd/prebuilds/reconcile.go | 2 +- enterprise/coderd/prebuilds/reconcile_test.go | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 3beb27acec5fd..2eeb907f0837b 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -947,7 +947,7 @@ func CompareGetRunningPrebuiltWorkspacesResults( // Convert optimized results to the same type as original for comparison var optimizedConverted []database.GetRunningPrebuiltWorkspacesRow if original != nil { - optimizedConverted := make([]database.GetRunningPrebuiltWorkspacesRow, len(optimized)) + optimizedConverted = make([]database.GetRunningPrebuiltWorkspacesRow, len(optimized)) for i, row := range optimized { optimizedConverted[i] = database.GetRunningPrebuiltWorkspacesRow(row) } diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index 7ead88a786b76..cd32c5ba64994 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -2397,9 +2397,13 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { // Should log exactly one error for count mismatch require.Len(t, spy.entries, 1) assert.Equal(t, slog.LevelError, spy.entries[0].Level) - assert.Equal(t, "result count mismatch for GetRunningPrebuiltWorkspacesOptimized", spy.entries[0].Message) - assert.Equal(t, 1, spy.getField(0, "original_count")) - assert.Equal(t, 2, spy.getField(0, "optimized_count")) + diff := spy.getField(0, "diff") + require.NotNil(t, diff) + diffStr := diff.(string) + + // The diff should contain information about the differences we introduced + assert.Contains(t, diffStr, "workspace2") // Original name + assert.Contains(t, diffStr, "CurrentPresetID") // Changed preset ID }) t.Run("field differences - logs errors", func(t *testing.T) { @@ -2428,7 +2432,6 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { // Should log exactly one error with a cmp.Diff output require.Len(t, spy.entries, 1) assert.Equal(t, slog.LevelError, spy.entries[0].Level) - assert.Equal(t, "GetRunningPrebuiltWorkspacesOptimized results differ from original", spy.entries[0].Message) diff := spy.getField(0, "diff") require.NotNil(t, diff) From 5dc69d94225e70822a212ea7574a3798a2abc018 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 3 Jul 2025 15:02:03 +0100 Subject: [PATCH 07/14] fixup! run both queries and diff results --- coderd/database/dbmock/dbmock.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 7b5a64f139b4f..49000e570d303 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -2748,21 +2748,6 @@ func (mr *MockStoreMockRecorder) GetRunningPrebuiltWorkspacesOptimized(ctx any) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRunningPrebuiltWorkspacesOptimized", reflect.TypeOf((*MockStore)(nil).GetRunningPrebuiltWorkspacesOptimized), ctx) } -// GetRunningPrebuiltWorkspacesWithComparison mocks base method. -func (m *MockStore) GetRunningPrebuiltWorkspacesWithComparison(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesRow, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetRunningPrebuiltWorkspacesWithComparison", ctx) - ret0, _ := ret[0].([]database.GetRunningPrebuiltWorkspacesRow) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetRunningPrebuiltWorkspacesWithComparison indicates an expected call of GetRunningPrebuiltWorkspacesWithComparison. -func (mr *MockStoreMockRecorder) GetRunningPrebuiltWorkspacesWithComparison(ctx any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRunningPrebuiltWorkspacesWithComparison", reflect.TypeOf((*MockStore)(nil).GetRunningPrebuiltWorkspacesWithComparison), ctx) -} - // GetRuntimeConfig mocks base method. func (m *MockStore) GetRuntimeConfig(ctx context.Context, key string) (string, error) { m.ctrl.T.Helper() From 5158046c66540301787f7498108c9fbee22760d5 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 3 Jul 2025 15:26:00 +0100 Subject: [PATCH 08/14] fixup! run both queries and diff results --- coderd/database/dbauthz/dbauthz_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index c94a049ed188f..4db36018f176a 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -179,7 +179,8 @@ func TestDBAuthzRecursive(t *testing.T) { if method.Name == "InTx" || method.Name == "Ping" || method.Name == "Wrappers" || - method.Name == "PGLocks" { + method.Name == "PGLocks" || + method.Name == "GetRunningPrebuiltWorkspacesOptimized" { continue } // easy to know which method failed. From 3e69a1bc282167b7635cfd57d268554999b402f6 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 3 Jul 2025 15:28:38 +0100 Subject: [PATCH 09/14] fixup! run both queries and diff results --- coderd/database/dbauthz/setup_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/database/dbauthz/setup_test.go b/coderd/database/dbauthz/setup_test.go index 555a17fb2070f..23effafc632e0 100644 --- a/coderd/database/dbauthz/setup_test.go +++ b/coderd/database/dbauthz/setup_test.go @@ -41,6 +41,8 @@ var skipMethods = map[string]string{ "Wrappers": "Not relevant", "AcquireLock": "Not relevant", "TryAcquireLock": "Not relevant", + // This method will be removed once we know this works correctly. + "GetRunningPrebuiltWorkspacesOptimized": "Not relevant", } // TestMethodTestSuite runs MethodTestSuite. From 2e760c71710e75a6d38e8ec5b96139dba8fa9c86 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 3 Jul 2025 18:13:08 +0100 Subject: [PATCH 10/14] address copilot comments --- coderd/database/dbmem/dbmem.go | 2 +- coderd/database/queries/prebuilds.sql | 4 +- enterprise/coderd/prebuilds/reconcile.go | 13 +++--- enterprise/coderd/prebuilds/reconcile_test.go | 42 +++++++++++++++++++ 4 files changed, 50 insertions(+), 11 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index bd0d87dd7fd18..30d29f4beb3a5 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -5102,7 +5102,7 @@ func (q *FakeQuerier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]datab } func (q *FakeQuerier) GetRunningPrebuiltWorkspacesOptimized(ctx context.Context) ([]database.GetRunningPrebuiltWorkspacesOptimizedRow, error) { - panic("not implemented") + return nil, ErrUnimplemented } func (q *FakeQuerier) GetRuntimeConfig(_ context.Context, key string) (string, error) { diff --git a/coderd/database/queries/prebuilds.sql b/coderd/database/queries/prebuilds.sql index 6d287205c11cc..0ca4d7a9b9650 100644 --- a/coderd/database/queries/prebuilds.sql +++ b/coderd/database/queries/prebuilds.sql @@ -96,8 +96,7 @@ SELECT FROM latest_prebuilds LEFT JOIN ready_agents ON ready_agents.job_id = latest_prebuilds.job_id LEFT JOIN workspace_latest_presets ON workspace_latest_presets.workspace_id = latest_prebuilds.id -ORDER BY latest_prebuilds.id -; +ORDER BY latest_prebuilds.id; -- name: GetRunningPrebuiltWorkspaces :many SELECT @@ -113,7 +112,6 @@ FROM workspace_prebuilds p WHERE (b.transition = 'start'::workspace_transition AND b.job_status = 'succeeded'::provisioner_job_status) ORDER BY p.id; -; -- name: CountInProgressPrebuilds :many -- CountInProgressPrebuilds returns the number of in-progress prebuilds, grouped by preset ID and transition. diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 2eeb907f0837b..9ce8f959e1bfa 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -13,7 +13,6 @@ import ( "time" "github.com/google/go-cmp/cmp" - "github.com/hashicorp/go-multierror" "github.com/prometheus/client_golang/prometheus" @@ -944,13 +943,13 @@ func CompareGetRunningPrebuiltWorkspacesResults( original []database.GetRunningPrebuiltWorkspacesRow, optimized []database.GetRunningPrebuiltWorkspacesOptimizedRow, ) { + if len(original) == 0 && len(optimized) == 0 { + return + } // Convert optimized results to the same type as original for comparison - var optimizedConverted []database.GetRunningPrebuiltWorkspacesRow - if original != nil { - optimizedConverted = make([]database.GetRunningPrebuiltWorkspacesRow, len(optimized)) - for i, row := range optimized { - optimizedConverted[i] = database.GetRunningPrebuiltWorkspacesRow(row) - } + optimizedConverted := make([]database.GetRunningPrebuiltWorkspacesRow, len(optimized)) + for i, row := range optimized { + optimizedConverted[i] = database.GetRunningPrebuiltWorkspacesRow(row) } // Compare the results and log an error if they differ. diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index cd32c5ba64994..3b94cdf63770e 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -2406,6 +2406,32 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { assert.Contains(t, diffStr, "CurrentPresetID") // Changed preset ID }) + t.Run("count mismatch - other direction", func(t *testing.T) { + t.Parallel() + + spy := &logSpy{} + logger := slog.Make(spy) + + original := []database.GetRunningPrebuiltWorkspacesRow{} + + optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{ + createOptimizedRow(createWorkspaceRow("550e8400-e29b-41d4-a716-446655440001", "workspace2", false)), + } + + prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) + + // Should log exactly one error for count mismatch + require.Len(t, spy.entries, 1) + assert.Equal(t, slog.LevelError, spy.entries[0].Level) + diff := spy.getField(0, "diff") + require.NotNil(t, diff) + diffStr := diff.(string) + + // The diff should contain information about the differences we introduced + assert.Contains(t, diffStr, "workspace2") // Original name + assert.Contains(t, diffStr, "CurrentPresetID") // Changed preset ID + }) + t.Run("field differences - logs errors", func(t *testing.T) { t.Parallel() @@ -2458,6 +2484,22 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { // Should not log any errors when both results are empty require.Empty(t, spy.entries) }) + + t.Run("nil original", func(t *testing.T) { + t.Parallel() + spy := &logSpy{} + logger := slog.Make(spy) + prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, nil, []database.GetRunningPrebuiltWorkspacesOptimizedRow{}) + require.Empty(t, spy.entries) + }) + + t.Run("nil optimized ", func(t *testing.T) { + t.Parallel() + spy := &logSpy{} + logger := slog.Make(spy) + prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, []database.GetRunningPrebuiltWorkspacesRow{}, nil) + require.Empty(t, spy.entries) + }) } // logSpy captures log entries for testing From a0f4f36f0685219c2d53ad034917b688a8824c2f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 7 Jul 2025 12:16:16 +0100 Subject: [PATCH 11/14] use sink instead of spy --- enterprise/coderd/prebuilds/reconcile_test.go | 116 +++++++----------- 1 file changed, 43 insertions(+), 73 deletions(-) diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index 3b94cdf63770e..858b01abc00b9 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "sort" + "strings" "sync" "testing" "time" @@ -26,6 +27,7 @@ import ( "tailscale.com/types/ptr" "cdr.dev/slog" + "cdr.dev/slog/sloggers/slogjson" "cdr.dev/slog/sloggers/slogtest" "github.com/coder/quartz" @@ -2358,8 +2360,8 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { t.Run("identical results - no logging", func(t *testing.T) { t.Parallel() - spy := &logSpy{} - logger := slog.Make(spy) + var sb strings.Builder + logger := slog.Make(slogjson.Sink(&sb)) original := []database.GetRunningPrebuiltWorkspacesRow{ createWorkspaceRow("550e8400-e29b-41d4-a716-446655440000", "workspace1", true), @@ -2374,14 +2376,14 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) // Should not log any errors when results are identical - require.Empty(t, spy.entries) + require.Empty(t, strings.TrimSpace(sb.String())) }) t.Run("count mismatch - logs error", func(t *testing.T) { t.Parallel() - spy := &logSpy{} - logger := slog.Make(spy) + var sb strings.Builder + logger := slog.Make(slogjson.Sink(&sb)) original := []database.GetRunningPrebuiltWorkspacesRow{ createWorkspaceRow("550e8400-e29b-41d4-a716-446655440000", "workspace1", true), @@ -2394,23 +2396,20 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) - // Should log exactly one error for count mismatch - require.Len(t, spy.entries, 1) - assert.Equal(t, slog.LevelError, spy.entries[0].Level) - diff := spy.getField(0, "diff") - require.NotNil(t, diff) - diffStr := diff.(string) - - // The diff should contain information about the differences we introduced - assert.Contains(t, diffStr, "workspace2") // Original name - assert.Contains(t, diffStr, "CurrentPresetID") // Changed preset ID + // Should log exactly one error. + if lines := strings.Split(strings.TrimSpace(sb.String()), "\n"); assert.NotEmpty(t, lines) { + require.Len(t, lines, 1) + assert.Contains(t, lines[0], "ERROR") + assert.Contains(t, lines[0], "workspace2") + assert.Contains(t, lines[0], "CurrentPresetID") + } }) t.Run("count mismatch - other direction", func(t *testing.T) { t.Parallel() - spy := &logSpy{} - logger := slog.Make(spy) + var sb strings.Builder + logger := slog.Make(slogjson.Sink(&sb)) original := []database.GetRunningPrebuiltWorkspacesRow{} @@ -2420,23 +2419,19 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) - // Should log exactly one error for count mismatch - require.Len(t, spy.entries, 1) - assert.Equal(t, slog.LevelError, spy.entries[0].Level) - diff := spy.getField(0, "diff") - require.NotNil(t, diff) - diffStr := diff.(string) - - // The diff should contain information about the differences we introduced - assert.Contains(t, diffStr, "workspace2") // Original name - assert.Contains(t, diffStr, "CurrentPresetID") // Changed preset ID + if lines := strings.Split(strings.TrimSpace(sb.String()), "\n"); assert.NotEmpty(t, lines) { + require.Len(t, lines, 1) + assert.Contains(t, lines[0], "ERROR") + assert.Contains(t, lines[0], "workspace2") + assert.Contains(t, lines[0], "CurrentPresetID") + } }) t.Run("field differences - logs errors", func(t *testing.T) { t.Parallel() - spy := &logSpy{} - logger := slog.Make(spy) + var sb strings.Builder + logger := slog.Make(slogjson.Sink(&sb)) workspace1 := createWorkspaceRow("550e8400-e29b-41d4-a716-446655440000", "workspace1", true) workspace2 := createWorkspaceRow("550e8400-e29b-41d4-a716-446655440001", "workspace2", false) @@ -2456,25 +2451,21 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) // Should log exactly one error with a cmp.Diff output - require.Len(t, spy.entries, 1) - assert.Equal(t, slog.LevelError, spy.entries[0].Level) - - diff := spy.getField(0, "diff") - require.NotNil(t, diff) - diffStr := diff.(string) - - // The diff should contain information about the differences we introduced - assert.Contains(t, diffStr, "different-name") // Changed name - assert.Contains(t, diffStr, "workspace1") // Original name - assert.Contains(t, diffStr, "Ready") // Changed ready status - assert.Contains(t, diffStr, "CurrentPresetID") // Changed preset ID + if lines := strings.Split(strings.TrimSpace(sb.String()), "\n"); assert.NotEmpty(t, lines) { + require.Len(t, lines, 1) + assert.Contains(t, lines[0], "ERROR") + assert.Contains(t, lines[0], "different-name") + assert.Contains(t, lines[0], "workspace1") + assert.Contains(t, lines[0], "Ready") + assert.Contains(t, lines[0], "CurrentPresetID") + } }) t.Run("empty results - no logging", func(t *testing.T) { t.Parallel() - spy := &logSpy{} - logger := slog.Make(spy) + var sb strings.Builder + logger := slog.Make(slogjson.Sink(&sb)) original := []database.GetRunningPrebuiltWorkspacesRow{} optimized := []database.GetRunningPrebuiltWorkspacesOptimizedRow{} @@ -2482,45 +2473,24 @@ func TestCompareGetRunningPrebuiltWorkspacesResults(t *testing.T) { prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, original, optimized) // Should not log any errors when both results are empty - require.Empty(t, spy.entries) + require.Empty(t, strings.TrimSpace(sb.String())) }) t.Run("nil original", func(t *testing.T) { t.Parallel() - spy := &logSpy{} - logger := slog.Make(spy) + var sb strings.Builder + logger := slog.Make(slogjson.Sink(&sb)) prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, nil, []database.GetRunningPrebuiltWorkspacesOptimizedRow{}) - require.Empty(t, spy.entries) + // Should not log any errors when original is nil + require.Empty(t, strings.TrimSpace(sb.String())) }) t.Run("nil optimized ", func(t *testing.T) { t.Parallel() - spy := &logSpy{} - logger := slog.Make(spy) + var sb strings.Builder + logger := slog.Make(slogjson.Sink(&sb)) prebuilds.CompareGetRunningPrebuiltWorkspacesResults(ctx, logger, []database.GetRunningPrebuiltWorkspacesRow{}, nil) - require.Empty(t, spy.entries) + // Should not log any errors when optimized is nil + require.Empty(t, strings.TrimSpace(sb.String())) }) } - -// logSpy captures log entries for testing -type logSpy struct { - entries []slog.SinkEntry -} - -func (s *logSpy) LogEntry(_ context.Context, e slog.SinkEntry) { - s.entries = append(s.entries, e) -} - -func (*logSpy) Sync() {} - -func (s *logSpy) getField(entryIndex int, fieldName string) interface{} { - if entryIndex >= len(s.entries) { - return nil - } - for _, field := range s.entries[entryIndex].Fields { - if field.Name == fieldName { - return field.Value - } - } - return nil -} From 30c86ccfc0485cad470d5f720094f822043c01b5 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 8 Jul 2025 12:28:02 +0100 Subject: [PATCH 12/14] add comments to new query CTEs --- coderd/database/queries.sql.go | 8 ++++++++ coderd/database/queries/prebuilds.sql | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 7d3ce70b46fe5..c5b917449a81e 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6502,6 +6502,11 @@ func (q *sqlQuerier) GetRunningPrebuiltWorkspaces(ctx context.Context) ([]GetRun const getRunningPrebuiltWorkspacesOptimized = `-- name: GetRunningPrebuiltWorkspacesOptimized :many WITH latest_prebuilds AS ( + -- All workspaces that match the following criteria: + -- 1. Owned by prebuilds user + -- 2. Not deleted + -- 3. Latest build is a 'start' transition + -- 4. Latest build was successful SELECT workspaces.id, workspaces.name, @@ -6517,6 +6522,8 @@ WITH latest_prebuilds AS ( AND NOT workspaces.deleted ), workspace_latest_presets AS ( + -- For each of the above workspaces, the preset_id of the most recent + -- successful start transition. SELECT DISTINCT ON (latest_prebuilds.id) latest_prebuilds.id AS workspace_id, workspace_builds.template_version_preset_id AS current_preset_id @@ -6527,6 +6534,7 @@ workspace_latest_presets AS ( ORDER BY latest_prebuilds.id, workspace_builds.build_number DESC ), ready_agents AS ( + -- For each of the above workspaces, check if all agents are ready. SELECT latest_prebuilds.job_id, BOOL_AND(workspace_agents.lifecycle_state = 'ready'::workspace_agent_lifecycle_state)::boolean AS ready diff --git a/coderd/database/queries/prebuilds.sql b/coderd/database/queries/prebuilds.sql index 0ca4d7a9b9650..7e1dbc71f4a26 100644 --- a/coderd/database/queries/prebuilds.sql +++ b/coderd/database/queries/prebuilds.sql @@ -50,6 +50,11 @@ WHERE tvp.desired_instances IS NOT NULL -- Consider only presets that have a pre -- name: GetRunningPrebuiltWorkspacesOptimized :many WITH latest_prebuilds AS ( + -- All workspaces that match the following criteria: + -- 1. Owned by prebuilds user + -- 2. Not deleted + -- 3. Latest build is a 'start' transition + -- 4. Latest build was successful SELECT workspaces.id, workspaces.name, @@ -65,6 +70,8 @@ WITH latest_prebuilds AS ( AND NOT workspaces.deleted ), workspace_latest_presets AS ( + -- For each of the above workspaces, the preset_id of the most recent + -- successful start transition. SELECT DISTINCT ON (latest_prebuilds.id) latest_prebuilds.id AS workspace_id, workspace_builds.template_version_preset_id AS current_preset_id @@ -75,6 +82,7 @@ workspace_latest_presets AS ( ORDER BY latest_prebuilds.id, workspace_builds.build_number DESC ), ready_agents AS ( + -- For each of the above workspaces, check if all agents are ready. SELECT latest_prebuilds.job_id, BOOL_AND(workspace_agents.lifecycle_state = 'ready'::workspace_agent_lifecycle_state)::boolean AS ready From 1d6b4c00c9fe02c8acc8bf514976ba3a70e380f3 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 8 Jul 2025 12:28:28 +0100 Subject: [PATCH 13/14] improve querier test --- coderd/database/dbgen/dbgen.go | 8 ++ coderd/database/querier_test.go | 147 +++++++++++--------------------- 2 files changed, 60 insertions(+), 95 deletions(-) diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index cb42a2d38904f..4081e7824dc7c 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -348,6 +348,14 @@ func Workspace(t testing.TB, db database.Store, orig database.WorkspaceTable) da NextStartAt: orig.NextStartAt, }) require.NoError(t, err, "insert workspace") + if orig.Deleted { + err = db.UpdateWorkspaceDeletedByID(genCtx, database.UpdateWorkspaceDeletedByIDParams{ + ID: workspace.ID, + Deleted: true, + }) + require.NoError(t, err, "set workspace as deleted") + workspace.Deleted = true + } return workspace } diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 3edee78fd311e..ec66e3d21bb95 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -5033,6 +5033,7 @@ func TestGetRunningPrebuiltWorkspaces(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) db, _ := dbtestutil.NewDB(t) + now := dbtime.Now() // Given: a prebuilt workspace with a successful start build and a stop build. org := dbgen.Organization(t, db, database.Organization{}) @@ -5051,115 +5052,71 @@ func TestGetRunningPrebuiltWorkspaces(t *testing.T) { DesiredInstances: sql.NullInt32{Int32: 1, Valid: true}, }) - // Create a prebuild workspace (owned by system user) - prebuildSystemUser := uuid.MustParse("c42fdf75-3097-471c-8c33-fb52454d81c0") - stoppedPrebuild := dbgen.Workspace(t, db, database.WorkspaceTable{ - OwnerID: prebuildSystemUser, - TemplateID: template.ID, - Name: "test-prebuild", - Deleted: false, - }) - - // Create a successful START build - stoppedPrebuildJob1 := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ - OrganizationID: org.ID, - InitiatorID: database.PrebuildsSystemUserID, - Provisioner: database.ProvisionerTypeEcho, - Type: database.ProvisionerJobTypeWorkspaceBuild, - StartedAt: sql.NullTime{Time: dbtime.Now().Add(-time.Minute), Valid: true}, - CompletedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, - Error: sql.NullString{}, - ErrorCode: sql.NullString{}, - }) - stoppedPrebuiltWorkspaceBuild1 := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ - WorkspaceID: stoppedPrebuild.ID, - TemplateVersionID: templateVersion.ID, - TemplateVersionPresetID: uuid.NullUUID{UUID: preset.ID, Valid: true}, - JobID: stoppedPrebuildJob1.ID, - BuildNumber: 1, - Transition: database.WorkspaceTransitionStart, - InitiatorID: database.PrebuildsSystemUserID, - Reason: database.BuildReasonInitiator, - }) - - // Create a STOP build (making this prebuild "not running") - stoppedPrebuildWorkspaceJob2 := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ - OrganizationID: org.ID, - InitiatorID: database.PrebuildsSystemUserID, - Provisioner: database.ProvisionerTypeEcho, - Type: database.ProvisionerJobTypeWorkspaceBuild, - StartedAt: sql.NullTime{Time: dbtime.Now().Add(-time.Minute), Valid: true}, - CompletedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, - Error: sql.NullString{}, - ErrorCode: sql.NullString{}, - }) - stoppedPrebuiltWorkspaceBuild2 := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ - WorkspaceID: stoppedPrebuild.ID, - TemplateVersionID: templateVersion.ID, - TemplateVersionPresetID: uuid.NullUUID{UUID: preset.ID, Valid: true}, - JobID: stoppedPrebuildWorkspaceJob2.ID, - BuildNumber: 2, - Transition: database.WorkspaceTransitionStop, - InitiatorID: database.PrebuildsSystemUserID, - Reason: database.BuildReasonInitiator, - }) - - // Create a second running prebuild workspace with a successful start build - // and no stop build. - runningPrebuild := dbgen.Workspace(t, db, database.WorkspaceTable{ - OwnerID: prebuildSystemUser, - TemplateID: template.ID, - Name: "test-running-prebuild", - Deleted: false, - }) - // Create a successful START build for the running prebuild workspace - runningPrebuildJob1 := dbgen.ProvisionerJob(t, db, nil, - database.ProvisionerJob{ + setupFixture := func(t *testing.T, db database.Store, name string, deleted bool, transition database.WorkspaceTransition, jobStatus database.ProvisionerJobStatus) database.WorkspaceTable { + t.Helper() + ws := dbgen.Workspace(t, db, database.WorkspaceTable{ + OwnerID: database.PrebuildsSystemUserID, + TemplateID: template.ID, + Name: name, + Deleted: deleted, + }) + var canceledAt sql.NullTime + var jobError sql.NullString + switch jobStatus { + case database.ProvisionerJobStatusFailed: + jobError = sql.NullString{String: assert.AnError.Error(), Valid: true} + case database.ProvisionerJobStatusCanceled: + canceledAt = sql.NullTime{Time: now, Valid: true} + } + pj := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ OrganizationID: org.ID, - InitiatorID: user.ID, + InitiatorID: database.PrebuildsSystemUserID, Provisioner: database.ProvisionerTypeEcho, Type: database.ProvisionerJobTypeWorkspaceBuild, - StartedAt: sql.NullTime{Time: dbtime.Now().Add(-time.Minute), Valid: true}, - CompletedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, - Error: sql.NullString{}, - ErrorCode: sql.NullString{}, - }) - runningPrebuiltWorkspaceBuild1 := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ - WorkspaceID: runningPrebuild.ID, - TemplateVersionID: templateVersion.ID, - TemplateVersionPresetID: uuid.NullUUID{UUID: preset.ID, Valid: true}, - JobID: runningPrebuildJob1.ID, - BuildNumber: 1, - Transition: database.WorkspaceTransitionStart, - InitiatorID: database.PrebuildsSystemUserID, - Reason: database.BuildReasonInitiator, - }) + StartedAt: sql.NullTime{Time: now.Add(-time.Minute), Valid: true}, + CanceledAt: canceledAt, + CompletedAt: sql.NullTime{Time: now, Valid: true}, + Error: jobError, + }) + wb := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: ws.ID, + TemplateVersionID: templateVersion.ID, + TemplateVersionPresetID: uuid.NullUUID{UUID: preset.ID, Valid: true}, + JobID: pj.ID, + BuildNumber: 1, + Transition: transition, + InitiatorID: database.PrebuildsSystemUserID, + Reason: database.BuildReasonInitiator, + }) + // Ensure things are set up as expectd + require.Equal(t, transition, wb.Transition) + require.Equal(t, int32(1), wb.BuildNumber) + require.Equal(t, jobStatus, pj.JobStatus) + require.Equal(t, deleted, ws.Deleted) + + return ws + } + + // Given: a number of prebuild workspaces with different states exist. + runningPrebuild := setupFixture(t, db, "running-prebuild", false, database.WorkspaceTransitionStart, database.ProvisionerJobStatusSucceeded) + _ = setupFixture(t, db, "stopped-prebuild", false, database.WorkspaceTransitionStop, database.ProvisionerJobStatusSucceeded) + _ = setupFixture(t, db, "failed-prebuild", false, database.WorkspaceTransitionStart, database.ProvisionerJobStatusFailed) + _ = setupFixture(t, db, "canceled-prebuild", false, database.WorkspaceTransitionStart, database.ProvisionerJobStatusCanceled) + _ = setupFixture(t, db, "deleted-prebuild", true, database.WorkspaceTransitionStart, database.ProvisionerJobStatusSucceeded) - // Create a third running regular workspace (not a prebuild) with a successful - // start build. - _ = dbgen.Workspace(t, db, database.WorkspaceTable{ + // Given: a regular workspace also exists. + _ = dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{ OwnerID: user.ID, TemplateID: template.ID, Name: "test-running-regular-workspace", Deleted: false, }) - // Given: assert test invariants. - require.Equal(t, database.WorkspaceTransitionStart, stoppedPrebuiltWorkspaceBuild1.Transition) - require.Equal(t, int32(1), stoppedPrebuiltWorkspaceBuild1.BuildNumber) - require.Equal(t, database.ProvisionerJobStatusSucceeded, stoppedPrebuildJob1.JobStatus) - require.Equal(t, database.WorkspaceTransitionStop, stoppedPrebuiltWorkspaceBuild2.Transition) - require.Equal(t, int32(2), stoppedPrebuiltWorkspaceBuild2.BuildNumber) - require.Equal(t, database.ProvisionerJobStatusSucceeded, stoppedPrebuildWorkspaceJob2.JobStatus) - require.Equal(t, database.WorkspaceTransitionStart, runningPrebuiltWorkspaceBuild1.Transition) - require.Equal(t, int32(1), runningPrebuiltWorkspaceBuild1.BuildNumber) - require.Equal(t, database.ProvisionerJobStatusSucceeded, runningPrebuildJob1.JobStatus) - // When: we query for running prebuild workspaces runningPrebuilds, err := db.GetRunningPrebuiltWorkspaces(ctx) require.NoError(t, err) - // Then: the stopped prebuild workspace should not be returned. + // Then: only the running prebuild workspace should be returned. require.Len(t, runningPrebuilds, 1, "expected only one running prebuilt workspace") require.Equal(t, runningPrebuild.ID, runningPrebuilds[0].ID, "expected the running prebuilt workspace to be returned") } From 6a1c635de218108d71d7a3cf254932f622bb519d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 8 Jul 2025 12:30:00 +0100 Subject: [PATCH 14/14] add TODO --- enterprise/coderd/prebuilds/reconcile.go | 1 + 1 file changed, 1 insertion(+) diff --git a/enterprise/coderd/prebuilds/reconcile.go b/enterprise/coderd/prebuilds/reconcile.go index 9ce8f959e1bfa..cce39ea251323 100644 --- a/enterprise/coderd/prebuilds/reconcile.go +++ b/enterprise/coderd/prebuilds/reconcile.go @@ -937,6 +937,7 @@ func SetPrebuildsReconciliationPaused(ctx context.Context, db database.Store, pa // CompareGetRunningPrebuiltWorkspacesResults compares the original and optimized // query results and logs any differences found. This function can be easily // removed once we're confident the optimized query works correctly. +// TODO(Cian): Remove this function once the optimized query is stable and correct. func CompareGetRunningPrebuiltWorkspacesResults( ctx context.Context, logger slog.Logger,