Skip to content

Commit 835049c

Browse files
mafredrimafredri
committed
refactor(coderd): drop sidebar app constraint and simplify provisionerdserver for tasks
Updates coder/internal#973 Updates coder/internal#974
1 parent 11f2411 commit 835049c

File tree

6 files changed

+94
-93
lines changed

6 files changed

+94
-93
lines changed

‎coderd/database/check_constraint.go‎

Lines changed: 10 additions & 11 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dump.sql‎

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000394_drop_workspace_build_ai_task_sidebar_app_id_required.down.sql‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
-- WARNING: Restoring this constraint after running a newer version of coderd
2+
-- and using tasks is bound to break this constraint.
3+
ALTER TABLE workspace_builds
4+
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‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
-- We no longer need to enforce this constraint as tasks have their own data
2+
-- model.
3+
ALTER TABLE workspace_builds
4+
DROP CONSTRAINT workspace_builds_ai_task_sidebar_app_id_required;

‎coderd/provisionerdserver/provisionerdserver.go‎

Lines changed: 23 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -2006,10 +2006,12 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
20062006
}
20072007
}
20082008

2009-
var taskAppID uuid.NullUUID
2010-
var taskAgentID uuid.NullUUID
2011-
var hasAITask bool
2012-
var warnUnknownTaskAppID bool
2009+
var (
2010+
hasAITask bool
2011+
unknownAppID string
2012+
taskAppID uuid.NullUUID
2013+
taskAgentID uuid.NullUUID
2014+
)
20132015
if tasks := jobType.WorkspaceBuild.GetAiTasks(); len(tasks) > 0 {
20142016
hasAITask = true
20152017
task := tasks[0]
@@ -2026,59 +2028,28 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
20262028
}
20272029

20282030
if !slices.Contains(appIDs, appID) {
2029-
warnUnknownTaskAppID = true
2030-
}
2031-
2032-
id, err := uuid.Parse(appID)
2033-
if err != nil {
2034-
return xerrors.Errorf("parse app id: %w", err)
2035-
}
2036-
2037-
taskAppID = uuid.NullUUID{UUID: id, Valid: true}
2038-
2039-
agentID, ok := agentIDByAppID[appID]
2040-
taskAgentID = uuid.NullUUID{UUID: agentID, Valid: ok}
2041-
}
2042-
2043-
// This is a hacky workaround for the issue with tasks 'disappearing' on stop:
2044-
// reuse has_ai_task and sidebar_app_id from the previous build.
2045-
// This workaround should be removed as soon as possible.
2046-
if workspaceBuild.Transition == database.WorkspaceTransitionStop && workspaceBuild.BuildNumber > 1 {
2047-
if prevBuild, err := s.Database.GetWorkspaceBuildByWorkspaceIDAndBuildNumber(ctx, database.GetWorkspaceBuildByWorkspaceIDAndBuildNumberParams{
2048-
WorkspaceID: workspaceBuild.WorkspaceID,
2049-
BuildNumber: workspaceBuild.BuildNumber - 1,
2050-
}); err == nil {
2051-
hasAITask = prevBuild.HasAITask.Bool
2052-
taskAppID = prevBuild.AITaskSidebarAppID
2053-
warnUnknownTaskAppID = false
2054-
s.Logger.Debug(ctx, "task workaround: reused has_ai_task and app_id from previous build to keep track of task",
2055-
slog.F("job_id", job.ID.String()),
2056-
slog.F("build_number", prevBuild.BuildNumber),
2057-
slog.F("workspace_id", workspace.ID),
2058-
slog.F("workspace_build_id", workspaceBuild.ID),
2059-
slog.F("transition", string(workspaceBuild.Transition)),
2060-
slog.F("sidebar_app_id", taskAppID.UUID),
2061-
slog.F("has_ai_task", hasAITask),
2062-
)
2031+
unknownAppID = appID
20632032
} else {
2064-
s.Logger.Error(ctx, "task workaround: tracking via has_ai_task and app_id from previous build failed",
2065-
slog.Error(err),
2066-
slog.F("job_id", job.ID.String()),
2067-
slog.F("workspace_id", workspace.ID),
2068-
slog.F("workspace_build_id", workspaceBuild.ID),
2069-
slog.F("transition", string(workspaceBuild.Transition)),
2070-
)
2033+
// Only parse for valid app and agent to avoid fk violation.
2034+
id, err := uuid.Parse(appID)
2035+
if err != nil {
2036+
return xerrors.Errorf("parse app id: %w", err)
2037+
}
2038+
taskAppID = uuid.NullUUID{UUID: id, Valid: true}
2039+
2040+
agentID, ok := agentIDByAppID[appID]
2041+
taskAgentID = uuid.NullUUID{UUID: agentID, Valid: ok}
20712042
}
20722043
}
20732044

2074-
if warnUnknownTaskAppID {
2045+
if unknownAppID != "" && workspaceBuild.Transition == database.WorkspaceTransitionStart {
20752046
// Ref: https://github.com/coder/coder/issues/18776
20762047
// This can happen for a number of reasons:
20772048
// 1. Misconfigured template
20782049
// 2. Count=0 on the agent due to stop transition, meaning the associated coder_app was not inserted.
20792050
// Failing the build at this point is not ideal, so log a warning instead.
20802051
s.Logger.Warn(ctx, "unknown ai_task_app_id",
2081-
slog.F("ai_task_app_id", taskAppID.UUID.String()),
2052+
slog.F("ai_task_app_id", unknownAppID),
20822053
slog.F("job_id", job.ID.String()),
20832054
slog.F("workspace_id", workspace.ID),
20842055
slog.F("workspace_build_id", workspaceBuild.ID),
@@ -2105,9 +2076,6 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
21052076
slog.F("transition", string(workspaceBuild.Transition)),
21062077
)
21072078
}
2108-
// Important: reset hasAITask and sidebarAppID so that we don't run into a fk constraint violation.
2109-
hasAITask = false
2110-
taskAppID = uuid.NullUUID{}
21112079
}
21122080

21132081
if hasAITask && workspaceBuild.Transition == database.WorkspaceTransitionStart {
@@ -2124,14 +2092,6 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
21242092
}
21252093
}
21262094

2127-
hasExternalAgent := false
2128-
for _, resource := range jobType.WorkspaceBuild.Resources {
2129-
if resource.Type == "coder_external_agent" {
2130-
hasExternalAgent = true
2131-
break
2132-
}
2133-
}
2134-
21352095
if task, err := db.GetTaskByWorkspaceID(ctx, workspace.ID); err == nil {
21362096
// Irrespective of whether the agent or sidebar app is present,
21372097
// perform the upsert to ensure a link between the task and
@@ -2153,20 +2113,21 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
21532113
return xerrors.Errorf("get task by workspace id: %w", err)
21542114
}
21552115

2156-
// Regardless of whether there is an AI task or not, update the field to indicate one way or the other since it
2157-
// always defaults to nil. ONLY if has_ai_task=true MUST ai_task_sidebar_app_id be set.
2116+
_, hasExternalAgent := slice.Find(jobType.WorkspaceBuild.Resources, func(resource *sdkproto.Resource) bool {
2117+
return resource.Type == "coder_external_agent"
2118+
})
21582119
if err := db.UpdateWorkspaceBuildFlagsByID(ctx, database.UpdateWorkspaceBuildFlagsByIDParams{
21592120
ID: workspaceBuild.ID,
21602121
HasAITask: sql.NullBool{
21612122
Bool: hasAITask,
21622123
Valid: true,
21632124
},
2125+
SidebarAppID: taskAppID, // SidebarAppID is not required, but kept for API backwards compatibility.
21642126
HasExternalAgent: sql.NullBool{
21652127
Bool: hasExternalAgent,
21662128
Valid: true,
21672129
},
2168-
SidebarAppID: taskAppID,
2169-
UpdatedAt: now,
2130+
UpdatedAt: now,
21702131
}); err != nil {
21712132
return xerrors.Errorf("update workspace build ai tasks and external agent flag: %w", err)
21722133
}

‎coderd/provisionerdserver/provisionerdserver_test.go‎

Lines changed: 53 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2864,11 +2864,12 @@ func TestCompleteJob(t *testing.T) {
28642864
input *proto.CompletedJob_WorkspaceBuild
28652865
isTask bool
28662866
expectTaskStatus database.TaskStatus
2867+
expectAppID uuid.NullUUID
28672868
expectHasAiTask bool
28682869
expectUsageEvent bool
28692870
}
28702871

2871-
sidebarAppID := uuid.NewString()
2872+
sidebarAppID := uuid.New()
28722873
for _, tc := range []testcase{
28732874
{
28742875
name: "has_ai_task is false by default",
@@ -2883,12 +2884,45 @@ func TestCompleteJob(t *testing.T) {
28832884
{
28842885
name: "has_ai_task is set to true",
28852886
transition: database.WorkspaceTransitionStart,
2887+
input: &proto.CompletedJob_WorkspaceBuild{
2888+
AiTasks: []*sdkproto.AITask{
2889+
{
2890+
Id: uuid.NewString(),
2891+
AppId: sidebarAppID.String(),
2892+
},
2893+
},
2894+
Resources: []*sdkproto.Resource{
2895+
{
2896+
Agents: []*sdkproto.Agent{
2897+
{
2898+
Id: uuid.NewString(),
2899+
Name: "a",
2900+
Apps: []*sdkproto.App{
2901+
{
2902+
Id: sidebarAppID.String(),
2903+
Slug: "test-app",
2904+
},
2905+
},
2906+
},
2907+
},
2908+
},
2909+
},
2910+
},
2911+
isTask: true,
2912+
expectTaskStatus: database.TaskStatusInitializing,
2913+
expectAppID: uuid.NullUUID{UUID: sidebarAppID, Valid: true},
2914+
expectHasAiTask: true,
2915+
expectUsageEvent: true,
2916+
},
2917+
{
2918+
name: "has_ai_task is set to true, with sidebar app id",
2919+
transition: database.WorkspaceTransitionStart,
28862920
input: &proto.CompletedJob_WorkspaceBuild{
28872921
AiTasks: []*sdkproto.AITask{
28882922
{
28892923
Id: uuid.NewString(),
28902924
SidebarApp: &sdkproto.AITaskSidebarApp{
2891-
Id: sidebarAppID,
2925+
Id: sidebarAppID.String(),
28922926
},
28932927
},
28942928
},
@@ -2900,7 +2934,7 @@ func TestCompleteJob(t *testing.T) {
29002934
Name: "a",
29012935
Apps: []*sdkproto.App{
29022936
{
2903-
Id: sidebarAppID,
2937+
Id: sidebarAppID.String(),
29042938
Slug: "test-app",
29052939
},
29062940
},
@@ -2911,6 +2945,7 @@ func TestCompleteJob(t *testing.T) {
29112945
},
29122946
isTask: true,
29132947
expectTaskStatus: database.TaskStatusInitializing,
2948+
expectAppID: uuid.NullUUID{UUID: sidebarAppID, Valid: true},
29142949
expectHasAiTask: true,
29152950
expectUsageEvent: true,
29162951
},
@@ -2922,28 +2957,25 @@ func TestCompleteJob(t *testing.T) {
29222957
AiTasks: []*sdkproto.AITask{
29232958
{
29242959
Id: uuid.NewString(),
2925-
SidebarApp: &sdkproto.AITaskSidebarApp{
2926-
// Non-existing app ID would previously trigger a FK violation.
2927-
Id: uuid.NewString(),
2928-
},
2960+
// Non-existing app ID would previously trigger a FK violation.
2961+
// Now it should just be ignored.
2962+
AppId: sidebarAppID.String(),
29292963
},
29302964
},
29312965
},
29322966
isTask: true,
29332967
expectTaskStatus: database.TaskStatusInitializing,
2934-
expectHasAiTask: false,
2935-
expectUsageEvent: false,
2968+
expectHasAiTask: true,
2969+
expectUsageEvent: true, // We no longer reset hasAITask if the app doesn't exist.
29362970
},
29372971
{
29382972
name: "has_ai_task is set to true, but transition is not start",
29392973
transition: database.WorkspaceTransitionStop,
29402974
input: &proto.CompletedJob_WorkspaceBuild{
29412975
AiTasks: []*sdkproto.AITask{
29422976
{
2943-
Id: uuid.NewString(),
2944-
SidebarApp: &sdkproto.AITaskSidebarApp{
2945-
Id: sidebarAppID,
2946-
},
2977+
Id: uuid.NewString(),
2978+
AppId: sidebarAppID.String(),
29472979
},
29482980
},
29492981
Resources: []*sdkproto.Resource{
@@ -2954,7 +2986,7 @@ func TestCompleteJob(t *testing.T) {
29542986
Name: "a",
29552987
Apps: []*sdkproto.App{
29562988
{
2957-
Id: sidebarAppID,
2989+
Id: sidebarAppID.String(),
29582990
Slug: "test-app",
29592991
},
29602992
},
@@ -2965,6 +2997,7 @@ func TestCompleteJob(t *testing.T) {
29652997
},
29662998
isTask: true,
29672999
expectTaskStatus: database.TaskStatusPaused,
3000+
expectAppID: uuid.NullUUID{UUID: sidebarAppID, Valid: true},
29683001
expectHasAiTask: true,
29693002
expectUsageEvent: false,
29703003
},
@@ -2978,7 +3011,7 @@ func TestCompleteJob(t *testing.T) {
29783011
},
29793012
isTask: true,
29803013
expectTaskStatus: database.TaskStatusPaused,
2981-
expectHasAiTask: true,
3014+
expectHasAiTask: false, // We no longer inherit this from the previous build.
29823015
expectUsageEvent: false,
29833016
},
29843017
} {
@@ -3092,15 +3125,16 @@ func TestCompleteJob(t *testing.T) {
30923125
require.True(t, build.HasAITask.Valid) // We ALWAYS expect a value to be set, therefore not nil, i.e. valid = true.
30933126
require.Equal(t, tc.expectHasAiTask, build.HasAITask.Bool)
30943127

3128+
task, err := db.GetTaskByID(ctx, genTask.ID)
30953129
if tc.isTask {
3096-
task, err := db.GetTaskByID(ctx, genTask.ID)
30973130
require.NoError(t, err)
30983131
require.Equal(t, tc.expectTaskStatus, task.Status)
3132+
} else {
3133+
require.Error(t, err)
30993134
}
31003135

3101-
if tc.expectHasAiTask && build.Transition != database.WorkspaceTransitionStop {
3102-
require.Equal(t, sidebarAppID, build.AITaskSidebarAppID.UUID.String())
3103-
}
3136+
require.Equal(t, tc.expectAppID, task.WorkspaceAppID)
3137+
require.Equal(t, tc.expectAppID, build.AITaskSidebarAppID)
31043138

31053139
if tc.expectUsageEvent {
31063140
// Check that a usage event was collected.

0 commit comments

Comments
 (0)