diff --git a/coderd/database/check_constraint.go b/coderd/database/check_constraint.go index 8b1917b7697db..c8752b207de16 100644 --- a/coderd/database/check_constraint.go +++ b/coderd/database/check_constraint.go @@ -6,15 +6,14 @@ type CheckConstraint string // CheckConstraint enums. const ( - CheckAPIKeysAllowListNotEmpty CheckConstraint = "api_keys_allow_list_not_empty" // api_keys - CheckOneTimePasscodeSet CheckConstraint = "one_time_passcode_set" // users - CheckUsersUsernameMinLength CheckConstraint = "users_username_min_length" // users - CheckMaxProvisionerLogsLength CheckConstraint = "max_provisioner_logs_length" // provisioner_jobs - CheckMaxLogsLength CheckConstraint = "max_logs_length" // workspace_agents - CheckSubsystemsNotNone CheckConstraint = "subsystems_not_none" // workspace_agents - CheckWorkspaceBuildsAiTaskSidebarAppIDRequired CheckConstraint = "workspace_builds_ai_task_sidebar_app_id_required" // workspace_builds - CheckWorkspaceBuildsDeadlineBelowMaxDeadline CheckConstraint = "workspace_builds_deadline_below_max_deadline" // workspace_builds - CheckTelemetryLockEventTypeConstraint CheckConstraint = "telemetry_lock_event_type_constraint" // telemetry_locks - CheckValidationMonotonicOrder CheckConstraint = "validation_monotonic_order" // template_version_parameters - CheckUsageEventTypeCheck CheckConstraint = "usage_event_type_check" // usage_events + CheckAPIKeysAllowListNotEmpty CheckConstraint = "api_keys_allow_list_not_empty" // api_keys + CheckOneTimePasscodeSet CheckConstraint = "one_time_passcode_set" // users + CheckUsersUsernameMinLength CheckConstraint = "users_username_min_length" // users + CheckMaxProvisionerLogsLength CheckConstraint = "max_provisioner_logs_length" // provisioner_jobs + CheckMaxLogsLength CheckConstraint = "max_logs_length" // workspace_agents + CheckSubsystemsNotNone CheckConstraint = "subsystems_not_none" // workspace_agents + CheckWorkspaceBuildsDeadlineBelowMaxDeadline CheckConstraint = "workspace_builds_deadline_below_max_deadline" // workspace_builds + CheckTelemetryLockEventTypeConstraint CheckConstraint = "telemetry_lock_event_type_constraint" // telemetry_locks + CheckValidationMonotonicOrder CheckConstraint = "validation_monotonic_order" // template_version_parameters + CheckUsageEventTypeCheck CheckConstraint = "usage_event_type_check" // usage_events ) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 8790bd27df693..f08454a89731d 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -1950,7 +1950,6 @@ CREATE TABLE workspace_builds ( has_ai_task boolean, ai_task_sidebar_app_id uuid, has_external_agent boolean, - CONSTRAINT workspace_builds_ai_task_sidebar_app_id_required CHECK (((((has_ai_task IS NULL) OR (has_ai_task = false)) AND (ai_task_sidebar_app_id IS NULL)) OR ((has_ai_task = true) AND (ai_task_sidebar_app_id IS NOT NULL)))), CONSTRAINT workspace_builds_deadline_below_max_deadline CHECK ((((deadline <> '0001-01-01 00:00:00+00'::timestamp with time zone) AND (deadline <= max_deadline)) OR (max_deadline = '0001-01-01 00:00:00+00'::timestamp with time zone))) ); diff --git a/coderd/database/migrations/000394_drop_workspace_build_ai_task_sidebar_app_id_required.down.sql b/coderd/database/migrations/000394_drop_workspace_build_ai_task_sidebar_app_id_required.down.sql new file mode 100644 index 0000000000000..c079189235a62 --- /dev/null +++ b/coderd/database/migrations/000394_drop_workspace_build_ai_task_sidebar_app_id_required.down.sql @@ -0,0 +1,4 @@ +-- WARNING: Restoring this constraint after running a newer version of coderd +-- and using tasks is bound to break this constraint. +ALTER TABLE workspace_builds +ADD CONSTRAINT workspace_builds_ai_task_sidebar_app_id_required CHECK (((((has_ai_task IS NULL) OR (has_ai_task = false)) AND (ai_task_sidebar_app_id IS NULL)) OR ((has_ai_task = true) AND (ai_task_sidebar_app_id IS NOT NULL)))); diff --git a/coderd/database/migrations/000394_drop_workspace_build_ai_task_sidebar_app_id_required.up.sql b/coderd/database/migrations/000394_drop_workspace_build_ai_task_sidebar_app_id_required.up.sql new file mode 100644 index 0000000000000..4703b6f764a56 --- /dev/null +++ b/coderd/database/migrations/000394_drop_workspace_build_ai_task_sidebar_app_id_required.up.sql @@ -0,0 +1,4 @@ +-- We no longer need to enforce this constraint as tasks have their own data +-- model. +ALTER TABLE workspace_builds +DROP CONSTRAINT workspace_builds_ai_task_sidebar_app_id_required; diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 2e00796d1cd64..7c96c36eab163 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -2006,10 +2006,12 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro } } - var taskAppID uuid.NullUUID - var taskAgentID uuid.NullUUID - var hasAITask bool - var warnUnknownTaskAppID bool + var ( + hasAITask bool + unknownAppID string + taskAppID uuid.NullUUID + taskAgentID uuid.NullUUID + ) if tasks := jobType.WorkspaceBuild.GetAiTasks(); len(tasks) > 0 { hasAITask = true task := tasks[0] @@ -2026,59 +2028,29 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro } if !slices.Contains(appIDs, appID) { - warnUnknownTaskAppID = true - } - - id, err := uuid.Parse(appID) - if err != nil { - return xerrors.Errorf("parse app id: %w", err) - } - - taskAppID = uuid.NullUUID{UUID: id, Valid: true} - - agentID, ok := agentIDByAppID[appID] - taskAgentID = uuid.NullUUID{UUID: agentID, Valid: ok} - } - - // This is a hacky workaround for the issue with tasks 'disappearing' on stop: - // reuse has_ai_task and sidebar_app_id from the previous build. - // This workaround should be removed as soon as possible. - if workspaceBuild.Transition == database.WorkspaceTransitionStop && workspaceBuild.BuildNumber > 1 { - if prevBuild, err := s.Database.GetWorkspaceBuildByWorkspaceIDAndBuildNumber(ctx, database.GetWorkspaceBuildByWorkspaceIDAndBuildNumberParams{ - WorkspaceID: workspaceBuild.WorkspaceID, - BuildNumber: workspaceBuild.BuildNumber - 1, - }); err == nil { - hasAITask = prevBuild.HasAITask.Bool - taskAppID = prevBuild.AITaskSidebarAppID - warnUnknownTaskAppID = false - s.Logger.Debug(ctx, "task workaround: reused has_ai_task and app_id from previous build to keep track of task", - slog.F("job_id", job.ID.String()), - slog.F("build_number", prevBuild.BuildNumber), - slog.F("workspace_id", workspace.ID), - slog.F("workspace_build_id", workspaceBuild.ID), - slog.F("transition", string(workspaceBuild.Transition)), - slog.F("sidebar_app_id", taskAppID.UUID), - slog.F("has_ai_task", hasAITask), - ) + unknownAppID = appID + hasAITask = false } else { - s.Logger.Error(ctx, "task workaround: tracking via has_ai_task and app_id from previous build failed", - slog.Error(err), - slog.F("job_id", job.ID.String()), - slog.F("workspace_id", workspace.ID), - slog.F("workspace_build_id", workspaceBuild.ID), - slog.F("transition", string(workspaceBuild.Transition)), - ) + // Only parse for valid app and agent to avoid fk violation. + id, err := uuid.Parse(appID) + if err != nil { + return xerrors.Errorf("parse app id: %w", err) + } + taskAppID = uuid.NullUUID{UUID: id, Valid: true} + + agentID, ok := agentIDByAppID[appID] + taskAgentID = uuid.NullUUID{UUID: agentID, Valid: ok} } } - if warnUnknownTaskAppID { + if unknownAppID != "" && workspaceBuild.Transition == database.WorkspaceTransitionStart { // Ref: https://github.com/coder/coder/issues/18776 // This can happen for a number of reasons: // 1. Misconfigured template // 2. Count=0 on the agent due to stop transition, meaning the associated coder_app was not inserted. // Failing the build at this point is not ideal, so log a warning instead. s.Logger.Warn(ctx, "unknown ai_task_app_id", - slog.F("ai_task_app_id", taskAppID.UUID.String()), + slog.F("ai_task_app_id", unknownAppID), slog.F("job_id", job.ID.String()), slog.F("workspace_id", workspace.ID), slog.F("workspace_build_id", workspaceBuild.ID), @@ -2105,9 +2077,6 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro slog.F("transition", string(workspaceBuild.Transition)), ) } - // Important: reset hasAITask and sidebarAppID so that we don't run into a fk constraint violation. - hasAITask = false - taskAppID = uuid.NullUUID{} } if hasAITask && workspaceBuild.Transition == database.WorkspaceTransitionStart { @@ -2124,14 +2093,6 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro } } - hasExternalAgent := false - for _, resource := range jobType.WorkspaceBuild.Resources { - if resource.Type == "coder_external_agent" { - hasExternalAgent = true - break - } - } - if task, err := db.GetTaskByWorkspaceID(ctx, workspace.ID); err == nil { // Irrespective of whether the agent or sidebar app is present, // perform the upsert to ensure a link between the task and @@ -2153,20 +2114,21 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro return xerrors.Errorf("get task by workspace id: %w", err) } - // Regardless of whether there is an AI task or not, update the field to indicate one way or the other since it - // always defaults to nil. ONLY if has_ai_task=true MUST ai_task_sidebar_app_id be set. + _, hasExternalAgent := slice.Find(jobType.WorkspaceBuild.Resources, func(resource *sdkproto.Resource) bool { + return resource.Type == "coder_external_agent" + }) if err := db.UpdateWorkspaceBuildFlagsByID(ctx, database.UpdateWorkspaceBuildFlagsByIDParams{ ID: workspaceBuild.ID, HasAITask: sql.NullBool{ Bool: hasAITask, Valid: true, }, + SidebarAppID: taskAppID, // SidebarAppID is not required, but kept for API backwards compatibility. HasExternalAgent: sql.NullBool{ Bool: hasExternalAgent, Valid: true, }, - SidebarAppID: taskAppID, - UpdatedAt: now, + UpdatedAt: now, }); err != nil { return xerrors.Errorf("update workspace build ai tasks and external agent flag: %w", err) } diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 8d55e1529289f..0959b1fa0d4bb 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -2864,11 +2864,12 @@ func TestCompleteJob(t *testing.T) { input *proto.CompletedJob_WorkspaceBuild isTask bool expectTaskStatus database.TaskStatus + expectAppID uuid.NullUUID expectHasAiTask bool expectUsageEvent bool } - sidebarAppID := uuid.NewString() + sidebarAppID := uuid.New() for _, tc := range []testcase{ { name: "has_ai_task is false by default", @@ -2883,12 +2884,45 @@ func TestCompleteJob(t *testing.T) { { name: "has_ai_task is set to true", transition: database.WorkspaceTransitionStart, + input: &proto.CompletedJob_WorkspaceBuild{ + AiTasks: []*sdkproto.AITask{ + { + Id: uuid.NewString(), + AppId: sidebarAppID.String(), + }, + }, + Resources: []*sdkproto.Resource{ + { + Agents: []*sdkproto.Agent{ + { + Id: uuid.NewString(), + Name: "a", + Apps: []*sdkproto.App{ + { + Id: sidebarAppID.String(), + Slug: "test-app", + }, + }, + }, + }, + }, + }, + }, + isTask: true, + expectTaskStatus: database.TaskStatusInitializing, + expectAppID: uuid.NullUUID{UUID: sidebarAppID, Valid: true}, + expectHasAiTask: true, + expectUsageEvent: true, + }, + { + name: "has_ai_task is set to true, with sidebar app id", + transition: database.WorkspaceTransitionStart, input: &proto.CompletedJob_WorkspaceBuild{ AiTasks: []*sdkproto.AITask{ { Id: uuid.NewString(), SidebarApp: &sdkproto.AITaskSidebarApp{ - Id: sidebarAppID, + Id: sidebarAppID.String(), }, }, }, @@ -2900,7 +2934,7 @@ func TestCompleteJob(t *testing.T) { Name: "a", Apps: []*sdkproto.App{ { - Id: sidebarAppID, + Id: sidebarAppID.String(), Slug: "test-app", }, }, @@ -2911,6 +2945,7 @@ func TestCompleteJob(t *testing.T) { }, isTask: true, expectTaskStatus: database.TaskStatusInitializing, + expectAppID: uuid.NullUUID{UUID: sidebarAppID, Valid: true}, expectHasAiTask: true, expectUsageEvent: true, }, @@ -2922,10 +2957,9 @@ func TestCompleteJob(t *testing.T) { AiTasks: []*sdkproto.AITask{ { Id: uuid.NewString(), - SidebarApp: &sdkproto.AITaskSidebarApp{ - // Non-existing app ID would previously trigger a FK violation. - Id: uuid.NewString(), - }, + // Non-existing app ID would previously trigger a FK violation. + // Now it should just be ignored. + AppId: sidebarAppID.String(), }, }, }, @@ -2940,10 +2974,8 @@ func TestCompleteJob(t *testing.T) { input: &proto.CompletedJob_WorkspaceBuild{ AiTasks: []*sdkproto.AITask{ { - Id: uuid.NewString(), - SidebarApp: &sdkproto.AITaskSidebarApp{ - Id: sidebarAppID, - }, + Id: uuid.NewString(), + AppId: sidebarAppID.String(), }, }, Resources: []*sdkproto.Resource{ @@ -2954,7 +2986,7 @@ func TestCompleteJob(t *testing.T) { Name: "a", Apps: []*sdkproto.App{ { - Id: sidebarAppID, + Id: sidebarAppID.String(), Slug: "test-app", }, }, @@ -2965,6 +2997,7 @@ func TestCompleteJob(t *testing.T) { }, isTask: true, expectTaskStatus: database.TaskStatusPaused, + expectAppID: uuid.NullUUID{UUID: sidebarAppID, Valid: true}, expectHasAiTask: true, expectUsageEvent: false, }, @@ -2978,7 +3011,7 @@ func TestCompleteJob(t *testing.T) { }, isTask: true, expectTaskStatus: database.TaskStatusPaused, - expectHasAiTask: true, + expectHasAiTask: false, // We no longer inherit this from the previous build. expectUsageEvent: false, }, } { @@ -3092,15 +3125,16 @@ func TestCompleteJob(t *testing.T) { require.True(t, build.HasAITask.Valid) // We ALWAYS expect a value to be set, therefore not nil, i.e. valid = true. require.Equal(t, tc.expectHasAiTask, build.HasAITask.Bool) + task, err := db.GetTaskByID(ctx, genTask.ID) if tc.isTask { - task, err := db.GetTaskByID(ctx, genTask.ID) require.NoError(t, err) require.Equal(t, tc.expectTaskStatus, task.Status) + } else { + require.Error(t, err) } - if tc.expectHasAiTask && build.Transition != database.WorkspaceTransitionStop { - require.Equal(t, sidebarAppID, build.AITaskSidebarAppID.UUID.String()) - } + require.Equal(t, tc.expectAppID, task.WorkspaceAppID) + require.Equal(t, tc.expectAppID, build.AITaskSidebarAppID) if tc.expectUsageEvent { // Check that a usage event was collected.