Skip to content

Commit 8202514

Browse files
kacpersawdeansheather
kacpersaw
and
deansheather
authored
feat!: add ability to cancel pending workspace build (coder#18713)
Closes coder#17791 This PR adds ability to cancel workspace builds that are in "pending" status. Breaking changes: - CancelWorkspaceBuild method in codersdk now accepts an optional request parameter API: - Added `expect_status` query parameter to the cancel workspace build endpoint - This parameter ensures the job hasn't changed state before canceling - API returns `412 Precondition Failed` if the job is not in the expected status - Valid values: `running` or `pending` - Wrapped the entire cancel method in a database transaction UI: - Added confirmation dialog to the `Cancel` button, since it's a destructive operation ![image](https://github.com/user-attachments/assets/437aa5f4-5669-45b6-82a0-e46f277114bf) ![image](https://github.com/user-attachments/assets/423b5cb1-a4fb-4a10-933b-c1c73f4b838c) - Enabled cancel action for pending workspaces (`expect_status=pending` is sent if workspace is in pending status) ![image](https://github.com/user-attachments/assets/32d35ff1-12e6-4f7b-9f6c-fde9da9de6cf) --------- Co-authored-by: Dean Sheather <dean@deansheather.com>
1 parent 2f42b64 commit 8202514

File tree

20 files changed

+555
-92
lines changed

20 files changed

+555
-92
lines changed

‎cli/provisionerjobs.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ func (r *RootCmd) provisionerJobsCancel() *serpent.Command {
166166
err = client.CancelTemplateVersion(ctx, ptr.NilToEmpty(job.Input.TemplateVersionID))
167167
case codersdk.ProvisionerJobTypeWorkspaceBuild:
168168
_, _ = fmt.Fprintf(inv.Stdout, "Canceling workspace build job %s...\n", job.ID)
169-
err = client.CancelWorkspaceBuild(ctx, ptr.NilToEmpty(job.Input.WorkspaceBuildID))
169+
err = client.CancelWorkspaceBuild(ctx, ptr.NilToEmpty(job.Input.WorkspaceBuildID), codersdk.CancelWorkspaceBuildParams{})
170170
}
171171
if err != nil {
172172
return xerrors.Errorf("cancel provisioner job: %w", err)

‎coderd/apidoc/docs.go‎

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

‎coderd/apidoc/swagger.json‎

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

‎coderd/database/dbauthz/dbauthz.go‎

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1182,6 +1182,27 @@ func (q *querier) customRoleCheck(ctx context.Context, role database.CustomRole)
11821182
return nil
11831183
}
11841184

1185+
func (q *querier) authorizeProvisionerJob(ctx context.Context, job database.ProvisionerJob) error {
1186+
switch job.Type {
1187+
case database.ProvisionerJobTypeWorkspaceBuild:
1188+
// Authorized call to get workspace build. If we can read the build, we
1189+
// can read the job.
1190+
_, err := q.GetWorkspaceBuildByJobID(ctx, job.ID)
1191+
if err != nil {
1192+
return xerrors.Errorf("fetch related workspace build: %w", err)
1193+
}
1194+
case database.ProvisionerJobTypeTemplateVersionDryRun, database.ProvisionerJobTypeTemplateVersionImport:
1195+
// Authorized call to get template version.
1196+
_, err := authorizedTemplateVersionFromJob(ctx, q, job)
1197+
if err != nil {
1198+
return xerrors.Errorf("fetch related template version: %w", err)
1199+
}
1200+
default:
1201+
return xerrors.Errorf("unknown job type: %q", job.Type)
1202+
}
1203+
return nil
1204+
}
1205+
11851206
func (q *querier) AcquireLock(ctx context.Context, id int64) error {
11861207
return q.db.AcquireLock(ctx, id)
11871208
}
@@ -2445,32 +2466,24 @@ func (q *querier) GetProvisionerJobByID(ctx context.Context, id uuid.UUID) (data
24452466
return database.ProvisionerJob{}, err
24462467
}
24472468

2448-
switch job.Type {
2449-
case database.ProvisionerJobTypeWorkspaceBuild:
2450-
// Authorized call to get workspace build. If we can read the build, we
2451-
// can read the job.
2452-
_, err := q.GetWorkspaceBuildByJobID(ctx, id)
2453-
if err != nil {
2454-
return database.ProvisionerJob{}, xerrors.Errorf("fetch related workspace build: %w", err)
2455-
}
2456-
case database.ProvisionerJobTypeTemplateVersionDryRun, database.ProvisionerJobTypeTemplateVersionImport:
2457-
// Authorized call to get template version.
2458-
_, err := authorizedTemplateVersionFromJob(ctx, q, job)
2459-
if err != nil {
2460-
return database.ProvisionerJob{}, xerrors.Errorf("fetch related template version: %w", err)
2461-
}
2462-
default:
2463-
return database.ProvisionerJob{}, xerrors.Errorf("unknown job type: %q", job.Type)
2469+
if err := q.authorizeProvisionerJob(ctx, job); err != nil {
2470+
return database.ProvisionerJob{}, err
24642471
}
24652472

24662473
return job, nil
24672474
}
24682475

24692476
func (q *querier) GetProvisionerJobByIDForUpdate(ctx context.Context, id uuid.UUID) (database.ProvisionerJob, error) {
2470-
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil {
2477+
job, err := q.db.GetProvisionerJobByIDForUpdate(ctx, id)
2478+
if err != nil {
24712479
return database.ProvisionerJob{}, err
24722480
}
2473-
return q.db.GetProvisionerJobByIDForUpdate(ctx, id)
2481+
2482+
if err := q.authorizeProvisionerJob(ctx, job); err != nil {
2483+
return database.ProvisionerJob{}, err
2484+
}
2485+
2486+
return job, nil
24742487
}
24752488

24762489
func (q *querier) GetProvisionerJobTimingsByJobID(ctx context.Context, jobID uuid.UUID) ([]database.ProvisionerJobTiming, error) {

‎coderd/database/dbauthz/dbauthz_test.go‎

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4655,8 +4655,59 @@ func (s *MethodTestSuite) TestSystemFunctions() {
46554655
VapidPrivateKey: "test",
46564656
}).Asserts(rbac.ResourceDeploymentConfig, policy.ActionUpdate)
46574657
}))
4658-
s.Run("GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) {
4659-
check.Args(uuid.New()).Asserts(rbac.ResourceProvisionerJobs, policy.ActionRead).Errors(sql.ErrNoRows)
4658+
s.Run("Build/GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) {
4659+
u := dbgen.User(s.T(), db, database.User{})
4660+
o := dbgen.Organization(s.T(), db, database.Organization{})
4661+
tpl := dbgen.Template(s.T(), db, database.Template{
4662+
OrganizationID: o.ID,
4663+
CreatedBy: u.ID,
4664+
})
4665+
w := dbgen.Workspace(s.T(), db, database.WorkspaceTable{
4666+
OwnerID: u.ID,
4667+
OrganizationID: o.ID,
4668+
TemplateID: tpl.ID,
4669+
})
4670+
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{
4671+
Type: database.ProvisionerJobTypeWorkspaceBuild,
4672+
})
4673+
tv := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{
4674+
TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true},
4675+
JobID: j.ID,
4676+
OrganizationID: o.ID,
4677+
CreatedBy: u.ID,
4678+
})
4679+
_ = dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{
4680+
JobID: j.ID,
4681+
WorkspaceID: w.ID,
4682+
TemplateVersionID: tv.ID,
4683+
})
4684+
check.Args(j.ID).Asserts(w, policy.ActionRead).Returns(j)
4685+
}))
4686+
s.Run("TemplateVersion/GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) {
4687+
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
4688+
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{
4689+
Type: database.ProvisionerJobTypeTemplateVersionImport,
4690+
})
4691+
tpl := dbgen.Template(s.T(), db, database.Template{})
4692+
v := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{
4693+
TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true},
4694+
JobID: j.ID,
4695+
})
4696+
check.Args(j.ID).Asserts(v.RBACObject(tpl), policy.ActionRead).Returns(j)
4697+
}))
4698+
s.Run("TemplateVersionDryRun/GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) {
4699+
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
4700+
tpl := dbgen.Template(s.T(), db, database.Template{})
4701+
v := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{
4702+
TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true},
4703+
})
4704+
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{
4705+
Type: database.ProvisionerJobTypeTemplateVersionDryRun,
4706+
Input: must(json.Marshal(struct {
4707+
TemplateVersionID uuid.UUID `json:"template_version_id"`
4708+
}{TemplateVersionID: v.ID})),
4709+
})
4710+
check.Args(j.ID).Asserts(v.RBACObject(tpl), policy.ActionRead).Returns(j)
46604711
}))
46614712
s.Run("HasTemplateVersionsWithAITask", s.Subtest(func(db database.Store, check *expects) {
46624713
check.Args().Asserts()

‎coderd/workspacebuilds.go‎

Lines changed: 91 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -581,10 +581,24 @@ func (api *API) notifyWorkspaceUpdated(
581581
// @Produce json
582582
// @Tags Builds
583583
// @Param workspacebuild path string true "Workspace build ID"
584+
// @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)
584585
// @Success 200 {object} codersdk.Response
585586
// @Router /workspacebuilds/{workspacebuild}/cancel [patch]
586587
func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Request) {
587588
ctx := r.Context()
589+
590+
var expectStatus database.ProvisionerJobStatus
591+
expectStatusParam := r.URL.Query().Get("expect_status")
592+
if expectStatusParam != "" {
593+
if expectStatusParam != "running" && expectStatusParam != "pending" {
594+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
595+
Message: fmt.Sprintf("Invalid expect_status %q. Only 'running' or 'pending' are allowed.", expectStatusParam),
596+
})
597+
return
598+
}
599+
expectStatus = database.ProvisionerJobStatus(expectStatusParam)
600+
}
601+
588602
workspaceBuild := httpmw.WorkspaceBuildParam(r)
589603
workspace, err := api.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID)
590604
if err != nil {
@@ -594,58 +608,78 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
594608
return
595609
}
596610

597-
valid, err := api.verifyUserCanCancelWorkspaceBuilds(ctx, httpmw.APIKey(r).UserID, workspace.TemplateID)
598-
if err != nil {
599-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
600-
Message: "Internal error verifying permission to cancel workspace build.",
601-
Detail: err.Error(),
602-
})
603-
return
604-
}
605-
if !valid {
606-
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
607-
Message: "User is not allowed to cancel workspace builds. Owner role is required.",
608-
})
609-
return
611+
code := http.StatusInternalServerError
612+
resp := codersdk.Response{
613+
Message: "Internal error canceling workspace build.",
610614
}
615+
err = api.Database.InTx(func(db database.Store) error {
616+
valid, err := verifyUserCanCancelWorkspaceBuilds(ctx, db, httpmw.APIKey(r).UserID, workspace.TemplateID, expectStatus)
617+
if err != nil {
618+
code = http.StatusInternalServerError
619+
resp.Message = "Internal error verifying permission to cancel workspace build."
620+
resp.Detail = err.Error()
611621

612-
job, err := api.Database.GetProvisionerJobByID(ctx, workspaceBuild.JobID)
613-
if err != nil {
614-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
615-
Message: "Internal error fetching provisioner job.",
616-
Detail: err.Error(),
617-
})
618-
return
619-
}
620-
if job.CompletedAt.Valid {
621-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
622-
Message: "Job has already completed!",
623-
})
624-
return
625-
}
626-
if job.CanceledAt.Valid {
627-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
628-
Message: "Job has already been marked as canceled!",
622+
return xerrors.Errorf("verify user can cancel workspace builds: %w", err)
623+
}
624+
if !valid {
625+
code = http.StatusForbidden
626+
resp.Message = "User is not allowed to cancel workspace builds. Owner role is required."
627+
628+
return xerrors.New("user is not allowed to cancel workspace builds")
629+
}
630+
631+
job, err := db.GetProvisionerJobByIDForUpdate(ctx, workspaceBuild.JobID)
632+
if err != nil {
633+
code = http.StatusInternalServerError
634+
resp.Message = "Internal error fetching provisioner job."
635+
resp.Detail = err.Error()
636+
637+
return xerrors.Errorf("get provisioner job: %w", err)
638+
}
639+
if job.CompletedAt.Valid {
640+
code = http.StatusBadRequest
641+
resp.Message = "Job has already completed!"
642+
643+
return xerrors.New("job has already completed")
644+
}
645+
if job.CanceledAt.Valid {
646+
code = http.StatusBadRequest
647+
resp.Message = "Job has already been marked as canceled!"
648+
649+
return xerrors.New("job has already been marked as canceled")
650+
}
651+
652+
if expectStatus != "" && job.JobStatus != expectStatus {
653+
code = http.StatusPreconditionFailed
654+
resp.Message = "Job is not in the expected state."
655+
656+
return xerrors.Errorf("job is not in the expected state: expected: %q, got %q", expectStatus, job.JobStatus)
657+
}
658+
659+
err = db.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{
660+
ID: job.ID,
661+
CanceledAt: sql.NullTime{
662+
Time: dbtime.Now(),
663+
Valid: true,
664+
},
665+
CompletedAt: sql.NullTime{
666+
Time: dbtime.Now(),
667+
// If the job is running, don't mark it completed!
668+
Valid: !job.WorkerID.Valid,
669+
},
629670
})
630-
return
631-
}
632-
err = api.Database.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{
633-
ID: job.ID,
634-
CanceledAt: sql.NullTime{
635-
Time: dbtime.Now(),
636-
Valid: true,
637-
},
638-
CompletedAt: sql.NullTime{
639-
Time: dbtime.Now(),
640-
// If the job is running, don't mark it completed!
641-
Valid: !job.WorkerID.Valid,
642-
},
643-
})
671+
if err != nil {
672+
code = http.StatusInternalServerError
673+
resp.Message = "Internal error updating provisioner job."
674+
resp.Detail = err.Error()
675+
676+
return xerrors.Errorf("update provisioner job: %w", err)
677+
}
678+
679+
return nil
680+
}, nil)
644681
if err != nil {
645-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
646-
Message: "Internal error updating provisioner job.",
647-
Detail: err.Error(),
648-
})
682+
httpapi.Write(ctx, rw, code, resp)
649683
return
650684
}
651685

@@ -659,8 +693,14 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
659693
})
660694
}
661695

662-
func (api *API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, userID uuid.UUID, templateID uuid.UUID) (bool, error) {
663-
template, err := api.Database.GetTemplateByID(ctx, templateID)
696+
func verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID, jobStatus database.ProvisionerJobStatus) (bool, error) {
697+
// If the jobStatus is pending, we always allow cancellation regardless of
698+
// the template setting as it's non-destructive to Terraform resources.
699+
if jobStatus == database.ProvisionerJobStatusPending {
700+
return true, nil
701+
}
702+
703+
template, err := store.GetTemplateByID(ctx, templateID)
664704
if err != nil {
665705
return false, xerrors.New("no template exists for this workspace")
666706
}
@@ -669,7 +709,7 @@ func (api *API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, userID u
669709
return true, nil // all users can cancel workspace builds
670710
}
671711

672-
user, err := api.Database.GetUserByID(ctx, userID)
712+
user, err := store.GetUserByID(ctx, userID)
673713
if err != nil {
674714
return false, xerrors.New("user does not exist")
675715
}

0 commit comments

Comments
 (0)