shishantbiswas/coder
1
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-06-02 20:48:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(coderd): drop sidebar app constraint and simplify provisionerdserver for tasks (#20591)

Browse Source 
Updates coder/internal#973
Updates coder/internal#974
This commit is contained in:
Mathias Fredriksson
2025-11-03 13:46:38 +02:00
committed by GitHub
parent 1961252918
commit a6b0eae38d
6 changed files with 93 additions and 91 deletions
Download Patch File Download Diff File Expand all files Collapse all files
coderd/database/check_constraint.go
+10 -11
View File
@@ -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
)
coderd/database/dump.sql
Generated
-1
View File
@@ -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)))
);
coderd/database/migrations/000394_drop_workspace_build_ai_task_sidebar_app_id_required.down.sql
+4
View File
@@ -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))));
coderd/database/migrations/000394_drop_workspace_build_ai_task_sidebar_app_id_required.up.sql
+4
View File
@@ -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;
coderd/provisionerdserver/provisionerdserver.go
+24 -62
View File
@@ -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)
}
coderd/provisionerdserver/provisionerdserver_test.go
+51 -17
View File
@@ -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.