Skip to content

feat!: add ability to cancel pending workspace build #18713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Jul 8, 2025
Merged

feat!: add ability to cancel pending workspace build #18713

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ba41ae8
add support for canceling workspace builds with expect_state param (l…
kacpersaw Jul 1, 2025
c4ee5b6
Use single transaction for canceling workspace build
kacpersaw Jul 2, 2025
c49c33e
Fix lint problem in ut
kacpersaw Jul 2, 2025
ba1dbf3
add cancel confirmation dialog for workspace builds and add expect_st…
kacpersaw Jul 2, 2025
acffda6
Fix lint
kacpersaw Jul 2, 2025
b672d76
Merge branch 'main' into kacpersaw/cancel-pending-provisioner-jobs
kacpersaw Jul 2, 2025
86a34df
Apply review suggestions
kacpersaw Jul 3, 2025
42170ab
Fix unit test
kacpersaw Jul 3, 2025
5db9d71
Apply review suggestions
kacpersaw Jul 3, 2025
1ede20c
Fix typo
kacpersaw Jul 3, 2025
2597615
Regenerate api types
kacpersaw Jul 3, 2025
6c2d0cf
Fix typo
kacpersaw Jul 3, 2025
c800494
Merge branch 'main' into kacpersaw/cancel-pending-provisioner-jobs
kacpersaw Jul 3, 2025
c5cb203
Apply a new authorization check for GetProvisionerJobByIDForUpdate
kacpersaw Jul 6, 2025
1de84cc
Apply a new authorization check for GetProvisionerJobByIDForUpdate
kacpersaw Jul 6, 2025
17fb6a3
Apply FE review suggestions
kacpersaw Jul 6, 2025
1b7b614
Apply review suggestions
kacpersaw Jul 7, 2025
634f556
Refactor cancelWorkspaceBuild parameter handling
kacpersaw Jul 7, 2025
4deace0
Fix lint
kacpersaw Jul 7, 2025
43430fa
Update coderd/workspacebuilds.go
kacpersaw Jul 7, 2025
6272d93
Extract cancel confirm dialog to a separate component
kacpersaw Jul 7, 2025
4d4a01d
Merge branch 'main' into kacpersaw/cancel-pending-provisioner-jobs
kacpersaw Jul 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Apply review suggestions
  • Loading branch information
@kacpersaw
kacpersaw committed Jul 3, 2025
commit 86a34dfba1557356fee2509a30555cf495ecc221
81 changes: 40 additions & 41 deletions coderd/workspacebuilds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,56 +596,58 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
return
}

code := http.StatusOK
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably have a default status code of 500 and a generic error message, otherwise if the tx returns an error below (because the tx couldn't start) you'll be returning an empty 200 response.

resp := codersdk.Response{}
err = api.Database.InTx(func(store database.Store) error {
valid, err := api.verifyUserCanCancelWorkspaceBuilds(ctx, store, httpmw.APIKey(r).UserID, workspace.TemplateID)
valid, err := verifyUserCanCancelWorkspaceBuilds(ctx, store, httpmw.APIKey(r).UserID, workspace.TemplateID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We often just use the variable name db in these transaction closures

if err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error verifying permission to cancel workspace build.",
Detail: err.Error(),
})
return nil
code = http.StatusInternalServerError
resp.Message = "Internal error verifying permission to cancel workspace build."
resp.Detail = err.Error()

return xerrors.Errorf("verify user can cancel workspace builds: %w", err)
}
if !valid {
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
Message: "User is not allowed to cancel workspace builds. Owner role is required.",
})
return nil
code = http.StatusForbidden
resp.Message = "User is not allowed to cancel workspace builds. Owner role is required."

return xerrors.Errorf("user is not allowed to cancel workspace builds")
}

job, err := store.GetProvisionerJobByID(ctx, workspaceBuild.JobID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a clone of this query with a SELECT ... FOR UPDATE query instead, otherwise there's nothing preventing this job from being picked up during the cancel operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I forgot to change that query 🙅

if err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error fetching provisioner job.",
Detail: err.Error(),
})
return 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 {
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "Job has already completed!",
})
return nil
code = http.StatusBadRequest
resp.Message = "Job has already completed!"

return xerrors.Errorf("job has already completed")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return xerrors.Errorf("job has already completed")
return xerrors.New("job has already completed")

}
if job.CanceledAt.Valid {
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "Job has already been marked as canceled!",
})
return nil
code = http.StatusBadRequest
resp.Message = "Job has already been marked as canceled!"

return xerrors.Errorf("job has already been marked as canceled")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return xerrors.Errorf("job has already been marked as canceled")
return xerrors.New("job has already been marked as canceled")

}

if expectStatus != "" {
if expectStatus != "running" && expectStatus != "pending" {
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "Invalid expect_status. Only 'running' or 'pending' are allowed.",
})
return nil
code = http.StatusBadRequest
resp.Message = "Invalid expect_status. Only 'running' or 'pending' are allowed."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resp.Message = "Invalid expect_status. Only 'running' or 'pending' are allowed."
resp.Message = fmt.Sprintf("Invalid expect_status %q. Only 'running' or 'pending' are allowed.", expectStatus)


return xerrors.Errorf("invalid expect_status")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return xerrors.Errorf("invalid expect_status")
return xerrors.Errorf("invalid expect_status %q", expectStatus)

}

if job.JobStatus != database.ProvisionerJobStatus(expectStatus) {
httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{
Message: "Job is not in the expected state.",
})
return nil
code = http.StatusPreconditionFailed
resp.Message = "Job is not in the expected state."

return xerrors.Errorf("job is not in the expected state")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return xerrors.Errorf("job is not in the expected state")
return xerrors.Errorf("job is not in the expected state: expected %q, got %q", expectStatus, job.JobStatus)

}
}

Expand All @@ -662,20 +664,17 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
},
})
if err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error updating provisioner job.",
Detail: err.Error(),
})
return 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
}

Expand All @@ -689,7 +688,7 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
})
}

func (*API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID) (bool, error) {
func verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID) (bool, error) {
template, err := store.GetTemplateByID(ctx, templateID)
if err != nil {
return false, xerrors.New("no template exists for this workspace")
Expand Down
3 changes: 1 addition & 2 deletions coderd/workspacebuilds_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,8 +763,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
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()
ctx := testutil.Context(t, testutil.WaitLong)

// When: a cancel request is made with invalid expect_state
err := client.CancelWorkspaceBuild(ctx, workspace.LatestBuild.ID, codersdk.CancelWorkspaceBuildRequest{
Expand Down
Loading