diff --git a/cli/provisionerjobs.go b/cli/provisionerjobs.go index c2b6b78658447..2ddd04c5b6a29 100644 --- a/cli/provisionerjobs.go +++ b/cli/provisionerjobs.go @@ -166,7 +166,7 @@ func (r *RootCmd) provisionerJobsCancel() *serpent.Command { err = client.CancelTemplateVersion(ctx, ptr.NilToEmpty(job.Input.TemplateVersionID)) case codersdk.ProvisionerJobTypeWorkspaceBuild: _, _ = fmt.Fprintf(inv.Stdout, "Canceling workspace build job %s...\n", job.ID) - err = client.CancelWorkspaceBuild(ctx, ptr.NilToEmpty(job.Input.WorkspaceBuildID)) + err = client.CancelWorkspaceBuild(ctx, ptr.NilToEmpty(job.Input.WorkspaceBuildID), codersdk.CancelWorkspaceBuildParams{}) } if err != nil { return xerrors.Errorf("cancel provisioner job: %w", err) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 1ee6ea77af5d9..433a3797daaaa 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -9122,6 +9122,16 @@ const docTemplate = `{ "name": "workspacebuild", "in": "path", "required": true + }, + { + "enum": [ + "running", + "pending" + ], + "type": "string", + "description": "Expected status of the job. If expect_status is supplied, the request will be rejected with 412 Precondition Failed if the job doesn't match the state when performing the cancellation.", + "name": "expect_status", + "in": "query" } ], "responses": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index b55a08caa8ec6..adac5bf344cbe 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -8065,6 +8065,13 @@ "name": "workspacebuild", "in": "path", "required": true + }, + { + "enum": ["running", "pending"], + "type": "string", + "description": "Expected status of the job. If expect_status is supplied, the request will be rejected with 412 Precondition Failed if the job doesn't match the state when performing the cancellation.", + "name": "expect_status", + "in": "query" } ], "responses": { diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index eea1b04a51fc5..b4162f0db59d7 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1182,6 +1182,27 @@ func (q *querier) customRoleCheck(ctx context.Context, role database.CustomRole) return nil } +func (q *querier) authorizeProvisionerJob(ctx context.Context, job database.ProvisionerJob) error { + switch job.Type { + case database.ProvisionerJobTypeWorkspaceBuild: + // Authorized call to get workspace build. If we can read the build, we + // can read the job. + _, err := q.GetWorkspaceBuildByJobID(ctx, job.ID) + if err != nil { + return xerrors.Errorf("fetch related workspace build: %w", err) + } + case database.ProvisionerJobTypeTemplateVersionDryRun, database.ProvisionerJobTypeTemplateVersionImport: + // Authorized call to get template version. + _, err := authorizedTemplateVersionFromJob(ctx, q, job) + if err != nil { + return xerrors.Errorf("fetch related template version: %w", err) + } + default: + return xerrors.Errorf("unknown job type: %q", job.Type) + } + return nil +} + func (q *querier) AcquireLock(ctx context.Context, id int64) error { return q.db.AcquireLock(ctx, id) } @@ -2445,32 +2466,24 @@ func (q *querier) GetProvisionerJobByID(ctx context.Context, id uuid.UUID) (data return database.ProvisionerJob{}, err } - switch job.Type { - case database.ProvisionerJobTypeWorkspaceBuild: - // Authorized call to get workspace build. If we can read the build, we - // can read the job. - _, err := q.GetWorkspaceBuildByJobID(ctx, id) - if err != nil { - return database.ProvisionerJob{}, xerrors.Errorf("fetch related workspace build: %w", err) - } - case database.ProvisionerJobTypeTemplateVersionDryRun, database.ProvisionerJobTypeTemplateVersionImport: - // Authorized call to get template version. - _, err := authorizedTemplateVersionFromJob(ctx, q, job) - if err != nil { - return database.ProvisionerJob{}, xerrors.Errorf("fetch related template version: %w", err) - } - default: - return database.ProvisionerJob{}, xerrors.Errorf("unknown job type: %q", job.Type) + if err := q.authorizeProvisionerJob(ctx, job); err != nil { + return database.ProvisionerJob{}, err } return job, nil } func (q *querier) GetProvisionerJobByIDForUpdate(ctx context.Context, id uuid.UUID) (database.ProvisionerJob, error) { - if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil { + job, err := q.db.GetProvisionerJobByIDForUpdate(ctx, id) + if err != nil { return database.ProvisionerJob{}, err } - return q.db.GetProvisionerJobByIDForUpdate(ctx, id) + + if err := q.authorizeProvisionerJob(ctx, job); err != nil { + return database.ProvisionerJob{}, err + } + + return job, nil } func (q *querier) GetProvisionerJobTimingsByJobID(ctx context.Context, jobID uuid.UUID) ([]database.ProvisionerJobTiming, error) { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 006320ef459a4..bcbe6bb881dde 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -4655,8 +4655,59 @@ func (s *MethodTestSuite) TestSystemFunctions() { VapidPrivateKey: "test", }).Asserts(rbac.ResourceDeploymentConfig, policy.ActionUpdate) })) - s.Run("GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) { - check.Args(uuid.New()).Asserts(rbac.ResourceProvisionerJobs, policy.ActionRead).Errors(sql.ErrNoRows) + s.Run("Build/GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) { + u := dbgen.User(s.T(), db, database.User{}) + o := dbgen.Organization(s.T(), db, database.Organization{}) + tpl := dbgen.Template(s.T(), db, database.Template{ + OrganizationID: o.ID, + CreatedBy: u.ID, + }) + w := dbgen.Workspace(s.T(), db, database.WorkspaceTable{ + OwnerID: u.ID, + OrganizationID: o.ID, + TemplateID: tpl.ID, + }) + j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{ + Type: database.ProvisionerJobTypeWorkspaceBuild, + }) + tv := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true}, + JobID: j.ID, + OrganizationID: o.ID, + CreatedBy: u.ID, + }) + _ = dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{ + JobID: j.ID, + WorkspaceID: w.ID, + TemplateVersionID: tv.ID, + }) + check.Args(j.ID).Asserts(w, policy.ActionRead).Returns(j) + })) + s.Run("TemplateVersion/GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) { + dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) + j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{ + Type: database.ProvisionerJobTypeTemplateVersionImport, + }) + tpl := dbgen.Template(s.T(), db, database.Template{}) + v := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true}, + JobID: j.ID, + }) + check.Args(j.ID).Asserts(v.RBACObject(tpl), policy.ActionRead).Returns(j) + })) + s.Run("TemplateVersionDryRun/GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) { + dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) + tpl := dbgen.Template(s.T(), db, database.Template{}) + v := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true}, + }) + j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{ + Type: database.ProvisionerJobTypeTemplateVersionDryRun, + Input: must(json.Marshal(struct { + TemplateVersionID uuid.UUID `json:"template_version_id"` + }{TemplateVersionID: v.ID})), + }) + check.Args(j.ID).Asserts(v.RBACObject(tpl), policy.ActionRead).Returns(j) })) s.Run("HasTemplateVersionsWithAITask", s.Subtest(func(db database.Store, check *expects) { check.Args().Asserts() diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 6c3321625c9b3..c8b1008280b09 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -581,10 +581,24 @@ func (api *API) notifyWorkspaceUpdated( // @Produce json // @Tags Builds // @Param workspacebuild path string true "Workspace build ID" +// @Param expect_status query string false "Expected status of the job. If expect_status is supplied, the request will be rejected with 412 Precondition Failed if the job doesn't match the state when performing the cancellation." Enums(running, pending) // @Success 200 {object} codersdk.Response // @Router /workspacebuilds/{workspacebuild}/cancel [patch] func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() + + var expectStatus database.ProvisionerJobStatus + expectStatusParam := r.URL.Query().Get("expect_status") + if expectStatusParam != "" { + if expectStatusParam != "running" && expectStatusParam != "pending" { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Invalid expect_status %q. Only 'running' or 'pending' are allowed.", expectStatusParam), + }) + return + } + expectStatus = database.ProvisionerJobStatus(expectStatusParam) + } + workspaceBuild := httpmw.WorkspaceBuildParam(r) workspace, err := api.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) if err != nil { @@ -594,58 +608,78 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques return } - valid, err := api.verifyUserCanCancelWorkspaceBuilds(ctx, httpmw.APIKey(r).UserID, workspace.TemplateID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error verifying permission to cancel workspace build.", - Detail: err.Error(), - }) - return - } - if !valid { - httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ - Message: "User is not allowed to cancel workspace builds. Owner role is required.", - }) - return + code := http.StatusInternalServerError + resp := codersdk.Response{ + Message: "Internal error canceling workspace build.", } + err = api.Database.InTx(func(db database.Store) error { + valid, err := verifyUserCanCancelWorkspaceBuilds(ctx, db, httpmw.APIKey(r).UserID, workspace.TemplateID, expectStatus) + if err != nil { + code = http.StatusInternalServerError + resp.Message = "Internal error verifying permission to cancel workspace build." + resp.Detail = err.Error() - job, err := api.Database.GetProvisionerJobByID(ctx, workspaceBuild.JobID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching provisioner job.", - Detail: err.Error(), - }) - return - } - if job.CompletedAt.Valid { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Job has already completed!", - }) - return - } - if job.CanceledAt.Valid { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Job has already been marked as canceled!", + return xerrors.Errorf("verify user can cancel workspace builds: %w", err) + } + if !valid { + code = http.StatusForbidden + resp.Message = "User is not allowed to cancel workspace builds. Owner role is required." + + return xerrors.New("user is not allowed to cancel workspace builds") + } + + job, err := db.GetProvisionerJobByIDForUpdate(ctx, workspaceBuild.JobID) + if err != nil { + code = http.StatusInternalServerError + resp.Message = "Internal error fetching provisioner job." + resp.Detail = err.Error() + + return xerrors.Errorf("get provisioner job: %w", err) + } + if job.CompletedAt.Valid { + code = http.StatusBadRequest + resp.Message = "Job has already completed!" + + return xerrors.New("job has already completed") + } + if job.CanceledAt.Valid { + code = http.StatusBadRequest + resp.Message = "Job has already been marked as canceled!" + + return xerrors.New("job has already been marked as canceled") + } + + if expectStatus != "" && job.JobStatus != expectStatus { + code = http.StatusPreconditionFailed + resp.Message = "Job is not in the expected state." + + return xerrors.Errorf("job is not in the expected state: expected: %q, got %q", expectStatus, job.JobStatus) + } + + err = db.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{ + ID: job.ID, + CanceledAt: sql.NullTime{ + Time: dbtime.Now(), + Valid: true, + }, + CompletedAt: sql.NullTime{ + Time: dbtime.Now(), + // If the job is running, don't mark it completed! + Valid: !job.WorkerID.Valid, + }, }) - return - } - err = api.Database.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{ - ID: job.ID, - CanceledAt: sql.NullTime{ - Time: dbtime.Now(), - Valid: true, - }, - CompletedAt: sql.NullTime{ - Time: dbtime.Now(), - // If the job is running, don't mark it completed! - Valid: !job.WorkerID.Valid, - }, - }) + if err != nil { + code = http.StatusInternalServerError + resp.Message = "Internal error updating provisioner job." + resp.Detail = err.Error() + + return xerrors.Errorf("update provisioner job: %w", err) + } + + return nil + }, nil) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error updating provisioner job.", - Detail: err.Error(), - }) + httpapi.Write(ctx, rw, code, resp) return } @@ -659,8 +693,14 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques }) } -func (api *API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, userID uuid.UUID, templateID uuid.UUID) (bool, error) { - template, err := api.Database.GetTemplateByID(ctx, templateID) +func verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID, jobStatus database.ProvisionerJobStatus) (bool, error) { + // If the jobStatus is pending, we always allow cancellation regardless of + // the template setting as it's non-destructive to Terraform resources. + if jobStatus == database.ProvisionerJobStatusPending { + return true, nil + } + + template, err := store.GetTemplateByID(ctx, templateID) if err != nil { return false, xerrors.New("no template exists for this workspace") } @@ -669,7 +709,7 @@ func (api *API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, userID u return true, nil // all users can cancel workspace builds } - user, err := api.Database.GetUserByID(ctx, userID) + user, err := store.GetUserByID(ctx, userID) if err != nil { return false, xerrors.New("user does not exist") } diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index b9d32a00b139a..ebab0770b71b4 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -573,7 +573,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { build, err = client.WorkspaceBuild(ctx, workspace.LatestBuild.ID) return assert.NoError(t, err) && build.Job.Status == codersdk.ProvisionerJobRunning }, testutil.WaitShort, testutil.IntervalFast) - err := client.CancelWorkspaceBuild(ctx, build.ID) + err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}) require.NoError(t, err) require.Eventually(t, func() bool { var err error @@ -618,11 +618,199 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { build, err = userClient.WorkspaceBuild(ctx, workspace.LatestBuild.ID) return assert.NoError(t, err) && build.Job.Status == codersdk.ProvisionerJobRunning }, testutil.WaitShort, testutil.IntervalFast) - err := userClient.CancelWorkspaceBuild(ctx, build.ID) + err := userClient.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) }) + + t.Run("Cancel with expect_state=pending", func(t *testing.T) { + t.Parallel() + if !dbtestutil.WillUsePostgres() { + t.Skip("this test requires postgres") + } + // Given: a coderd instance with a provisioner daemon + store, ps, db := dbtestutil.NewDBWithSQLDB(t) + client, closeDaemon := coderdtest.NewWithProvisionerCloser(t, &coderdtest.Options{ + Database: store, + Pubsub: ps, + IncludeProvisionerDaemon: true, + }) + defer closeDaemon.Close() + // Given: a user, template, and workspace + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + + // Stop the provisioner daemon. + require.NoError(t, closeDaemon.Close()) + ctx := testutil.Context(t, testutil.WaitLong) + // Given: no provisioner daemons exist. + _, err := db.ExecContext(ctx, `DELETE FROM provisioner_daemons;`) + require.NoError(t, err) + + // When: a new workspace build is created + build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: template.ActiveVersionID, + Transition: codersdk.WorkspaceTransitionStart, + }) + // Then: the request should succeed. + require.NoError(t, err) + // Then: the provisioner job should remain pending. + require.Equal(t, codersdk.ProvisionerJobPending, build.Job.Status) + + // Then: the response should indicate no provisioners are available. + if assert.NotNil(t, build.MatchedProvisioners) { + assert.Zero(t, build.MatchedProvisioners.Count) + assert.Zero(t, build.MatchedProvisioners.Available) + assert.Zero(t, build.MatchedProvisioners.MostRecentlySeen.Time) + assert.False(t, build.MatchedProvisioners.MostRecentlySeen.Valid) + } + + // When: the workspace build is canceled + err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{ + ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending, + }) + require.NoError(t, err) + + // Then: the workspace build should be canceled. + build, err = client.WorkspaceBuild(ctx, build.ID) + require.NoError(t, err) + require.Equal(t, codersdk.ProvisionerJobCanceled, build.Job.Status) + }) + + t.Run("Cancel with expect_state=pending when job is running - should fail with 412", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: []*proto.Response{{ + Type: &proto.Response_Log{ + Log: &proto.Log{}, + }, + }}, + ProvisionPlan: echo.PlanComplete, + }) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, template.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + var build codersdk.WorkspaceBuild + require.Eventually(t, func() bool { + var err error + build, err = client.WorkspaceBuild(ctx, workspace.LatestBuild.ID) + return assert.NoError(t, err) && build.Job.Status == codersdk.ProvisionerJobRunning + }, testutil.WaitShort, testutil.IntervalFast) + + // When: a cancel request is made with expect_state=pending + err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{ + ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending, + }) + // Then: the request should fail with 412. + require.Error(t, err) + + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusPreconditionFailed, apiErr.StatusCode()) + }) + + t.Run("Cancel with expect_state=running when job is pending - should fail with 412", func(t *testing.T) { + t.Parallel() + if !dbtestutil.WillUsePostgres() { + t.Skip("this test requires postgres") + } + // Given: a coderd instance with a provisioner daemon + store, ps, db := dbtestutil.NewDBWithSQLDB(t) + client, closeDaemon := coderdtest.NewWithProvisionerCloser(t, &coderdtest.Options{ + Database: store, + Pubsub: ps, + IncludeProvisionerDaemon: true, + }) + defer closeDaemon.Close() + // Given: a user, template, and workspace + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + + // Stop the provisioner daemon. + require.NoError(t, closeDaemon.Close()) + ctx := testutil.Context(t, testutil.WaitLong) + // Given: no provisioner daemons exist. + _, err := db.ExecContext(ctx, `DELETE FROM provisioner_daemons;`) + require.NoError(t, err) + + // When: a new workspace build is created + build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: template.ActiveVersionID, + Transition: codersdk.WorkspaceTransitionStart, + }) + // Then: the request should succeed. + require.NoError(t, err) + // Then: the provisioner job should remain pending. + require.Equal(t, codersdk.ProvisionerJobPending, build.Job.Status) + + // Then: the response should indicate no provisioners are available. + if assert.NotNil(t, build.MatchedProvisioners) { + assert.Zero(t, build.MatchedProvisioners.Count) + assert.Zero(t, build.MatchedProvisioners.Available) + assert.Zero(t, build.MatchedProvisioners.MostRecentlySeen.Time) + assert.False(t, build.MatchedProvisioners.MostRecentlySeen.Valid) + } + + // When: a cancel request is made with expect_state=running + err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{ + ExpectStatus: codersdk.CancelWorkspaceBuildStatusRunning, + }) + // Then: the request should fail with 412. + require.Error(t, err) + + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusPreconditionFailed, apiErr.StatusCode()) + }) + + t.Run("Cancel with expect_state - invalid status", func(t *testing.T) { + t.Parallel() + + // Given: a coderd instance with a provisioner daemon + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: []*proto.Response{{ + Type: &proto.Response_Log{ + Log: &proto.Log{}, + }, + }}, + ProvisionPlan: echo.PlanComplete, + }) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, template.ID) + + ctx := testutil.Context(t, testutil.WaitLong) + + // When: a cancel request is made with invalid expect_state + err := client.CancelWorkspaceBuild(ctx, workspace.LatestBuild.ID, codersdk.CancelWorkspaceBuildParams{ + ExpectStatus: "invalid_status", + }) + // Then: the request should fail with 400. + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "Invalid expect_status") + }) } func TestWorkspaceBuildResources(t *testing.T) { @@ -968,7 +1156,7 @@ func TestWorkspaceBuildStatus(t *testing.T) { _ = closeDaemon.Close() // after successful cancel is "canceled" build = coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStart) - err = client.CancelWorkspaceBuild(ctx, build.ID) + err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}) require.NoError(t, err) workspace, err = client.Workspace(ctx, workspace.ID) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index d51a228a3f7a1..3a50f850da5e7 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -3245,7 +3245,7 @@ func TestWorkspaceWatcher(t *testing.T) { closeFunc.Close() build := coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStart) wait("first is for the workspace build itself", nil) - err = client.CancelWorkspaceBuild(ctx, build.ID) + err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}) require.NoError(t, err) wait("second is for the build cancel", nil) } diff --git a/codersdk/toolsdk/toolsdk_test.go b/codersdk/toolsdk/toolsdk_test.go index d08191a614a99..09b919a428a84 100644 --- a/codersdk/toolsdk/toolsdk_test.go +++ b/codersdk/toolsdk/toolsdk_test.go @@ -164,7 +164,7 @@ func TestTools(t *testing.T) { // Important: cancel the build. We don't run any provisioners, so this // will remain in the 'pending' state indefinitely. - require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID)) + require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID, codersdk.CancelWorkspaceBuildParams{})) }) t.Run("Start", func(t *testing.T) { @@ -184,7 +184,7 @@ func TestTools(t *testing.T) { // Important: cancel the build. We don't run any provisioners, so this // will remain in the 'pending' state indefinitely. - require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID)) + require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID, codersdk.CancelWorkspaceBuildParams{})) }) t.Run("TemplateVersionChange", func(t *testing.T) { @@ -216,7 +216,7 @@ func TestTools(t *testing.T) { require.Equal(t, r.Workspace.ID.String(), updateBuild.WorkspaceID.String()) require.Equal(t, newVersion.TemplateVersion.ID.String(), updateBuild.TemplateVersionID.String()) // Cancel the build so it doesn't remain in the 'pending' state indefinitely. - require.NoError(t, client.CancelWorkspaceBuild(ctx, updateBuild.ID)) + require.NoError(t, client.CancelWorkspaceBuild(ctx, updateBuild.ID, codersdk.CancelWorkspaceBuildParams{})) // Roll back to the original version rollbackBuild, err := testTool(t, toolsdk.CreateWorkspaceBuild, tb, toolsdk.CreateWorkspaceBuildArgs{ @@ -229,7 +229,7 @@ func TestTools(t *testing.T) { require.Equal(t, r.Workspace.ID.String(), rollbackBuild.WorkspaceID.String()) require.Equal(t, originalVersionID.String(), rollbackBuild.TemplateVersionID.String()) // Cancel the build so it doesn't remain in the 'pending' state indefinitely. - require.NoError(t, client.CancelWorkspaceBuild(ctx, rollbackBuild.ID)) + require.NoError(t, client.CancelWorkspaceBuild(ctx, rollbackBuild.ID, codersdk.CancelWorkspaceBuildParams{})) }) }) diff --git a/codersdk/workspacebuilds.go b/codersdk/workspacebuilds.go index 328b8bc26566f..4066341773994 100644 --- a/codersdk/workspacebuilds.go +++ b/codersdk/workspacebuilds.go @@ -123,9 +123,29 @@ func (c *Client) WorkspaceBuild(ctx context.Context, id uuid.UUID) (WorkspaceBui return workspaceBuild, json.NewDecoder(res.Body).Decode(&workspaceBuild) } +type CancelWorkspaceBuildStatus string + +const ( + CancelWorkspaceBuildStatusRunning CancelWorkspaceBuildStatus = "running" + CancelWorkspaceBuildStatusPending CancelWorkspaceBuildStatus = "pending" +) + +type CancelWorkspaceBuildParams struct { + // ExpectStatus ensures the build is in the expected status before canceling. + ExpectStatus CancelWorkspaceBuildStatus `json:"expect_status,omitempty"` +} + +func (c *CancelWorkspaceBuildParams) asRequestOption() RequestOption { + return func(r *http.Request) { + q := r.URL.Query() + q.Set("expect_status", string(c.ExpectStatus)) + r.URL.RawQuery = q.Encode() + } +} + // CancelWorkspaceBuild marks a workspace build job as canceled. -func (c *Client) CancelWorkspaceBuild(ctx context.Context, id uuid.UUID) error { - res, err := c.Request(ctx, http.MethodPatch, fmt.Sprintf("/api/v2/workspacebuilds/%s/cancel", id), nil) +func (c *Client) CancelWorkspaceBuild(ctx context.Context, id uuid.UUID, req CancelWorkspaceBuildParams) error { + res, err := c.Request(ctx, http.MethodPatch, fmt.Sprintf("/api/v2/workspacebuilds/%s/cancel", id), nil, req.asRequestOption()) if err != nil { return err } diff --git a/docs/reference/api/builds.md b/docs/reference/api/builds.md index bb279f5825f6e..686f19316a8c0 100644 --- a/docs/reference/api/builds.md +++ b/docs/reference/api/builds.md @@ -491,9 +491,17 @@ curl -X PATCH http://coder-server:8080/api/v2/workspacebuilds/{workspacebuild}/c ### Parameters -| Name | In | Type | Required | Description | -|------------------|------|--------|----------|--------------------| -| `workspacebuild` | path | string | true | Workspace build ID | +| Name | In | Type | Required | Description | +|------------------|-------|--------|----------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `workspacebuild` | path | string | true | Workspace build ID | +| `expect_status` | query | string | false | Expected status of the job. If expect_status is supplied, the request will be rejected with 412 Precondition Failed if the job doesn't match the state when performing the cancellation. | + +#### Enumerated Values + +| Parameter | Value | +|-----------------|-----------| +| `expect_status` | `running` | +| `expect_status` | `pending` | ### Example responses diff --git a/scaletest/workspacebuild/run.go b/scaletest/workspacebuild/run.go index f19c556823faf..3b369a4e48a72 100644 --- a/scaletest/workspacebuild/run.go +++ b/scaletest/workspacebuild/run.go @@ -150,7 +150,7 @@ func (r *CleanupRunner) Run(ctx context.Context, _ string, logs io.Writer) error if err == nil && build.Job.Status.Active() { // mark the build as canceled logger.Info(ctx, "canceling workspace build", slog.F("build_id", build.ID), slog.F("workspace_id", r.workspaceID)) - if err = r.client.CancelWorkspaceBuild(ctx, build.ID); err == nil { + if err = r.client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}); err == nil { // Wait for the job to cancel before we delete it _ = waitForBuild(ctx, logs, r.client, build.ID) // it will return a "build canceled" error } else { diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 2b13c77faffa1..dd8d3d77998d2 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1277,9 +1277,12 @@ class ApiMethods { cancelWorkspaceBuild = async ( workspaceBuildId: TypesGen.WorkspaceBuild["id"], + params?: TypesGen.CancelWorkspaceBuildParams, ): Promise => { const response = await this.axios.patch( `/api/v2/workspacebuilds/${workspaceBuildId}/cancel`, + null, + { params }, ); return response.data; diff --git a/site/src/api/queries/workspaces.ts b/site/src/api/queries/workspaces.ts index 5a4cdb46dd4e9..05fb09314d741 100644 --- a/site/src/api/queries/workspaces.ts +++ b/site/src/api/queries/workspaces.ts @@ -266,7 +266,12 @@ export const startWorkspace = ( export const cancelBuild = (workspace: Workspace, queryClient: QueryClient) => { return { mutationFn: () => { - return API.cancelWorkspaceBuild(workspace.latest_build.id); + const { status } = workspace.latest_build; + const params = + status === "pending" || status === "running" + ? { expect_status: status } + : undefined; + return API.cancelWorkspaceBuild(workspace.latest_build.id, params); }, onSuccess: async () => { await queryClient.invalidateQueries({ diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 4ab5403081a60..399b79499edd9 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -292,6 +292,19 @@ export const BypassRatelimitHeader = "X-Coder-Bypass-Ratelimit"; // From codersdk/client.go export const CLITelemetryHeader = "Coder-CLI-Telemetry"; +// From codersdk/workspacebuilds.go +export interface CancelWorkspaceBuildParams { + readonly expect_status?: CancelWorkspaceBuildStatus; +} + +// From codersdk/workspacebuilds.go +export type CancelWorkspaceBuildStatus = "pending" | "running"; + +export const CancelWorkspaceBuildStatuses: CancelWorkspaceBuildStatus[] = [ + "pending", + "running", +]; + // From codersdk/users.go export interface ChangePasswordWithOneTimePasscodeRequest { readonly email: string; diff --git a/site/src/modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog.tsx b/site/src/modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog.tsx new file mode 100644 index 0000000000000..cbd7eb7d0c1ed --- /dev/null +++ b/site/src/modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog.tsx @@ -0,0 +1,32 @@ +import type { Workspace } from "api/typesGenerated"; +import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog"; +import type { FC } from "react"; + +interface WorkspaceBuildCancelDialogProps { + open: boolean; + onClose: () => void; + onConfirm: () => void; + workspace: Workspace; +} + +export const WorkspaceBuildCancelDialog: FC< + WorkspaceBuildCancelDialogProps +> = ({ open, onClose, onConfirm, workspace }) => { + const action = + workspace.latest_build.status === "pending" + ? "remove the current build from the build queue" + : "stop the current build process"; + + return ( + + ); +}; diff --git a/site/src/modules/workspaces/actions.ts b/site/src/modules/workspaces/actions.ts index f109c4d9ad1b9..8b17d3e937c74 100644 --- a/site/src/modules/workspaces/actions.ts +++ b/site/src/modules/workspaces/actions.ts @@ -145,7 +145,7 @@ export const abilitiesByWorkspaceStatus = ( case "pending": { return { actions: ["pending"], - canCancel: false, + canCancel: true, canAcceptJobs: false, }; } diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index ad320018da9fb..645c03380501a 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -19,6 +19,7 @@ import { MockFailedWorkspace, MockOrganization, MockOutdatedWorkspace, + MockPendingWorkspace, MockStartingWorkspace, MockStoppedWorkspace, MockTemplate, @@ -224,11 +225,59 @@ describe("WorkspacePage", () => { }), ); + const user = userEvent.setup({ delay: 0 }); const cancelWorkspaceMock = jest .spyOn(API, "cancelWorkspaceBuild") .mockImplementation(() => Promise.resolve({ message: "job canceled" })); + await renderWorkspacePage(MockStartingWorkspace); + + // Click on Cancel + const cancelButton = await screen.findByRole("button", { name: "Cancel" }); + await user.click(cancelButton); + + // Get dialog and confirm + const dialog = await screen.findByTestId("dialog"); + const confirmButton = within(dialog).getByRole("button", { + name: "Confirm", + hidden: false, + }); + await user.click(confirmButton); + + expect(cancelWorkspaceMock).toHaveBeenCalledWith( + MockStartingWorkspace.latest_build.id, + undefined, + ); + }); - await testButton(MockStartingWorkspace, "Cancel", cancelWorkspaceMock); + it("requests cancellation when the user presses Cancel and the workspace is pending", async () => { + server.use( + http.get("/api/v2/users/:userId/workspace/:workspaceName", () => { + return HttpResponse.json(MockPendingWorkspace); + }), + ); + + const user = userEvent.setup({ delay: 0 }); + const cancelWorkspaceMock = jest + .spyOn(API, "cancelWorkspaceBuild") + .mockImplementation(() => Promise.resolve({ message: "job canceled" })); + await renderWorkspacePage(MockPendingWorkspace); + + // Click on Cancel + const cancelButton = await screen.findByRole("button", { name: "Cancel" }); + await user.click(cancelButton); + + // Get dialog and confirm + const dialog = await screen.findByTestId("dialog"); + const confirmButton = within(dialog).getByRole("button", { + name: "Confirm", + hidden: false, + }); + await user.click(confirmButton); + + expect(cancelWorkspaceMock).toHaveBeenCalledWith( + MockPendingWorkspace.latest_build.id, + { expect_status: "pending" }, + ); }); it("requests an update when the user presses Update", async () => { diff --git a/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx b/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx index 79ec5ad11a2b5..4034cc144e127 100644 --- a/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx @@ -20,6 +20,7 @@ import { displayError } from "components/GlobalSnackbar/utils"; import { useWorkspaceBuildLogs } from "hooks/useWorkspaceBuildLogs"; import { EphemeralParametersDialog } from "modules/workspaces/EphemeralParametersDialog/EphemeralParametersDialog"; import { WorkspaceErrorDialog } from "modules/workspaces/ErrorDialog/WorkspaceErrorDialog"; +import { WorkspaceBuildCancelDialog } from "modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog"; import { WorkspaceUpdateDialogs, useWorkspaceUpdate, @@ -80,6 +81,8 @@ export const WorkspaceReadyPage: FC = ({ ephemeralParameters: TypesGen.TemplateVersionParameter[]; }>({ open: false, action: "start", ephemeralParameters: [] }); + const [isCancelConfirmOpen, setIsCancelConfirmOpen] = useState(false); + const { mutate: mutateRestartWorkspace, isPending: isRestarting } = useMutation({ mutationFn: API.restartWorkspace, @@ -316,7 +319,7 @@ export const WorkspaceReadyPage: FC = ({ } }} handleUpdate={workspaceUpdate.update} - handleCancel={cancelBuildMutation.mutate} + handleCancel={() => setIsCancelConfirmOpen(true)} handleRetry={handleRetry} handleDebug={handleDebug} handleDormantActivate={async () => { @@ -352,6 +355,16 @@ export const WorkspaceReadyPage: FC = ({ } /> + setIsCancelConfirmOpen(false)} + onConfirm={() => { + cancelBuildMutation.mutate(); + setIsCancelConfirmOpen(false); + }} + workspace={workspace} + /> + diff --git a/site/src/pages/WorkspacesPage/WorkspacesTable.tsx b/site/src/pages/WorkspacesPage/WorkspacesTable.tsx index 6213224dea602..23695d8fbc591 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesTable.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesTable.tsx @@ -62,6 +62,7 @@ import { import { useAppLink } from "modules/apps/useAppLink"; import { useDashboard } from "modules/dashboard/useDashboard"; import { WorkspaceAppStatus } from "modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus"; +import { WorkspaceBuildCancelDialog } from "modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog"; import { WorkspaceDormantBadge } from "modules/workspaces/WorkspaceDormantBadge/WorkspaceDormantBadge"; import { WorkspaceMoreActions } from "modules/workspaces/WorkspaceMoreActions/WorkspaceMoreActions"; import { WorkspaceOutdatedTooltip } from "modules/workspaces/WorkspaceOutdatedTooltip/WorkspaceOutdatedTooltip"; @@ -495,8 +496,8 @@ const WorkspaceActionsCell: FC = ({ onError: onActionError, }); - // State for stop confirmation dialog const [isStopConfirmOpen, setIsStopConfirmOpen] = useState(false); + const [isCancelConfirmOpen, setIsCancelConfirmOpen] = useState(false); const isRetrying = startWorkspaceMutation.isPending || @@ -606,7 +607,7 @@ const WorkspaceActionsCell: FC = ({ {abilities.canCancel && ( setIsCancelConfirmOpen(true)} isLoading={cancelBuildMutation.isPending} label="Cancel build" > @@ -643,6 +644,16 @@ const WorkspaceActionsCell: FC = ({ }} type="delete" /> + + setIsCancelConfirmOpen(false)} + onConfirm={() => { + cancelBuildMutation.mutate(); + setIsCancelConfirmOpen(false); + }} + workspace={workspace} + /> ); };