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

kacpersaw · 2025-07-02T12:51:13Z

Closes #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
Enabled cancel action for pending workspaces (expect_status=pending is sent if workspace is in pending status)

…ocking)

…atus for pending builds

ethanndickson

Looking good, just one comment w/ the transaction

coderd/workspacebuilds.go

coderd/workspacebuilds_test.go

deansheather

BE only review

deansheather · 2025-07-03T14:05:30Z

coderd/workspacebuilds.go

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


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

deansheather · 2025-07-03T14:12:56Z

coderd/workspacebuilds.go

+		if !valid {
+			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")
+		}


I think we should still allow for cancellations even if the template forbids them as long as the user specified expect_status=pending.

deansheather · 2025-07-03T14:17:27Z

coderd/workspacebuilds.go

+			return xerrors.Errorf("user is not allowed to cancel workspace builds")
+		}
+
+		job, err := store.GetProvisionerJobByID(ctx, workspaceBuild.JobID)


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.

Good catch, I forgot to change that query 🙅

deansheather · 2025-07-03T14:17:39Z

coderd/workspacebuilds.go

+			code = http.StatusBadRequest
+			resp.Message = "Job has already completed!"
+
+			return xerrors.Errorf("job has already completed")


Suggested change

return xerrors.Errorf("job has already completed")

return xerrors.New("job has already completed")

deansheather · 2025-07-03T14:17:46Z

coderd/workspacebuilds.go

+			code = http.StatusBadRequest
+			resp.Message = "Job has already been marked as canceled!"
+
+			return xerrors.Errorf("job has already been marked as canceled")


Suggested change

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

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

deansheather · 2025-07-03T14:18:17Z

coderd/workspacebuilds.go

+				code = http.StatusBadRequest
+				resp.Message = "Invalid expect_status. Only 'running' or 'pending' are allowed."
+
+				return xerrors.Errorf("invalid expect_status")


Suggested change

return xerrors.Errorf("invalid expect_status")

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

deansheather · 2025-07-03T14:18:30Z

coderd/workspacebuilds.go

+		if expectStatus != "" {
+			if expectStatus != "running" && expectStatus != "pending" {
+				code = http.StatusBadRequest
+				resp.Message = "Invalid expect_status. Only 'running' or 'pending' are allowed."


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)

deansheather · 2025-07-03T14:18:58Z

coderd/workspacebuilds.go

+				code = http.StatusPreconditionFailed
+				resp.Message = "Job is not in the expected state."
+
+				return xerrors.Errorf("job is not in the expected state")


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)

deansheather · 2025-07-03T14:25:37Z

codersdk/workspacebuilds.go

+	CancelWorkspaceBuildStatusPending CancelWorkspaceBuildStatus = "pending"
+)
+
+type CancelWorkspaceBuildRequest struct {


Maybe call this Params instead of Request since it's not the request body

BrunoQuaresma

I left a few comments related to the FE.

Note: When submitting FE related code, it is always nice to include a screenshot or a short video demo showing off the changes. It makes easier for us to visualize things and make faster reviews.

BrunoQuaresma · 2025-07-03T17:48:11Z

site/src/api/api.ts

@@ -1277,9 +1277,14 @@ class ApiMethods {

 	cancelWorkspaceBuild = async (
 		workspaceBuildId: TypesGen.WorkspaceBuild["id"],
+		request?: TypesGen.CancelWorkspaceBuildParams,


Since the type has the "Params" sufix, I would name this variable as params instead of request.

BrunoQuaresma · 2025-07-03T17:48:43Z

site/src/api/api.ts

 	): Promise<TypesGen.Response> => {
+		const params = request?.expect_status


You can do:

const params = new URLSearchParams(params)

BrunoQuaresma · 2025-07-03T17:49:43Z

site/src/api/api.ts

 		const response = await this.axios.patch(
-			`/api/v2/workspacebuilds/${workspaceBuildId}/cancel`,
+			`/api/v2/workspacebuilds/${workspaceBuildId}/cancel${params ? `?${params}` : ""}`,


And just use it directly:

`/api/v2/workspacebuilds/${workspaceBuildId}/cancel?${params}`

BrunoQuaresma · 2025-07-03T17:51:27Z

site/src/api/queries/workspaces.ts

@@ -266,6 +266,12 @@ export const startWorkspace = (
 export const cancelBuild = (workspace: Workspace, queryClient: QueryClient) => {
 	return {
 		mutationFn: () => {
+			// If workspace status is pending, include expect_status parameter


The comment is just saying what the code is doing below. I would rather not having this comment at all, since I can see it in the code, or explaining the why we need to set the expect_status to pending.

BrunoQuaresma · 2025-07-03T17:52:19Z

site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx

@@ -352,6 +354,21 @@ export const WorkspaceReadyPage: FC<WorkspaceReadyPageProps> = ({
 				}
 			/>

+			{/* Cancel confirmation dialog */}


I don't think we need this comment. The code below is pretty easy to understand.

BrunoQuaresma · 2025-07-03T17:52:51Z

site/src/pages/WorkspacesPage/WorkspacesTable.tsx

@@ -497,6 +497,8 @@ const WorkspaceActionsCell: FC<WorkspaceActionsCellProps> = ({

 	// State for stop confirmation dialog
 	const [isStopConfirmOpen, setIsStopConfirmOpen] = useState(false);
+	// State for cancel confirmation dialog


The same here, even seeing we do this for the state above.

BrunoQuaresma · 2025-07-03T17:56:58Z

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;


I can be wrong, but I think when we add a comment to the struct, or its props in codersdk package, it gets included in the TS types as well. I think lefting a comment in the CancelWorkspaceBuildParams.ExpectStatus could be helpful to understand when and why use it.

I left a comment in codersdk, but unfortunately it doesn't get passed to the TS types 😞

deansheather

BG looks good, just some minor comments

deansheather · 2025-07-07T08:12:32Z

coderd/workspacebuilds.go

@@ -581,10 +581,12 @@ 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" Enums(running, pending)


It might pay to have a description field (idk if our API generation has something like that, you might need to check other routes) that explains in more detail what this actually does:

If expect_status is supplied, the request will be rejected with 499 Precondition Failed if the job doesn't match the state when performing the cancellation.

deansheather · 2025-07-07T08:13:41Z

coderd/workspacebuilds.go

+			if expectStatus != "running" && expectStatus != "pending" {
+				code = http.StatusBadRequest
+				resp.Message = fmt.Sprintf("Invalid expect_status %q. Only 'running' or 'pending' are allowed.", expectStatus)
+
+				return xerrors.Errorf("invalid expect_status %q", expectStatus)
+			}


This status type checking stuff should probably happen outside of the TX at the top of the route since it doesn't depend on the job properties.

deansheather · 2025-07-07T08:14:49Z

coderd/workspacebuilds.go

-		})
-		return
-	}
+	code := http.StatusOK


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.

coderd/workspacebuilds.go

deansheather · 2025-07-07T08:17:23Z

coderd/workspacebuilds.go

@@ -659,8 +688,13 @@ 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, expectStatus string) (bool, error) {


If you move the parsing/checking of the expectStatus param to the top of the route, this could just directly accept the database enum type instead of the string.

deansheather · 2025-07-07T08:33:30Z

coderd/workspacebuilds_test.go

+			return assert.NoError(t, err) && build.Job.Status == codersdk.ProvisionerJobRunning
+		}, testutil.WaitShort, testutil.IntervalFast)
+
+		// When: a cancel request is made with expect_state=pending


Does this have a chance to race against the job succeeding? The code for checking if the build is already done is before the code that checks the expect_status param

From my understanding - this job should stay in running state until cancel, because the test doesn't include ApplyComplete in ProvisionApply.
https://github.com/coder/coder/blob/main/provisioner/echo/serve.go#L194

coderd/workspacebuilds_test.go

deansheather · 2025-07-07T08:34:45Z

codersdk/workspacebuilds.go

+
+type CancelWorkspaceBuildParams struct {
+	// ExpectStatus ensures the build is in the expected status before canceling.
+	ExpectStatus CancelWorkspaceBuildStatus `json:"expect_status,omitempty"`


Suggested change

ExpectStatus CancelWorkspaceBuildStatus `json:"expect_status,omitempty"`

ExpectStatus CancelWorkspaceBuildStatus `json:"-"`

Since it's never used as JSON

Or just omit the json tag entirely perhaps

json tag is required to generate correct field names for TS - without it, you'll get ExpectStatus instead of the required expect_status param.

site/src/api/queries/workspaces.ts

outdated

deansheather

Backend looks great!

coderd/workspacebuilds.go

deansheather · 2025-07-07T11:00:35Z

site/src/pages/WorkspacesPage/WorkspacesTable.tsx

+			<ConfirmDialog
+				open={isCancelConfirmOpen}
+				title="Cancel workspace build"
+				description={`Are you sure you want to cancel the build for workspace "${workspace.name}"? This will stop the current build process.`}


Would be nice if this could differ based on whether it thinks it's going to cancel a pending or running build.

e.g. This will remove the current build from the build queue.

Same with the Dialog on the other page too, maybe it should be made into a component?

Co-authored-by: Dean Sheather <dean@deansheather.com>

BrunoQuaresma

Just left one minor question, but it should not block the PR. 👍

BrunoQuaresma · 2025-07-07T16:42:42Z

site/src/api/queries/workspaces.ts

+				status === "pending" || status === "running"
+					? { expect_status: status }
+					: undefined;
+			return API.cancelWorkspaceBuild(workspace.latest_build.id, params);


Double checking... can cancelWorkspaceBuild being called in any status? I see the params can be undefined, but it would still call the cancelWorkspaceBuild 🤔. This part of the code is quite hard for me to understand why the params are only set when the status is pending or running.

expect_status is an optional parameter for cancel that ensures the job is in a specific state before proceeding - it only supports pending and running. So, cancel can be called without it.

kacpersaw added 4 commits July 1, 2025 12:31

add support for canceling workspace builds with expect_state param (l…

ba41ae8

…ocking)

Use single transaction for canceling workspace build

c4ee5b6

Fix lint problem in ut

c49c33e

add cancel confirmation dialog for workspace builds and add expect_st…

ba1dbf3

…atus for pending builds

kacpersaw requested review from deansheather and ethanndickson July 2, 2025 12:51

github-actions bot assigned kacpersaw Jul 2, 2025

kacpersaw added 2 commits July 2, 2025 12:55

Fix lint

acffda6

Merge branch 'main' into kacpersaw/cancel-pending-provisioner-jobs

b672d76

kacpersaw marked this pull request as ready for review July 2, 2025 13:52

github-actions bot added the release/breaking This label is applied to PRs to detect breaking changes as part of the release process label Jul 2, 2025

ethanndickson reviewed Jul 3, 2025

View reviewed changes

coderd/workspacebuilds.go Outdated Show resolved Hide resolved

coderd/workspacebuilds.go Outdated Show resolved Hide resolved

coderd/workspacebuilds_test.go Outdated Show resolved Hide resolved

Apply review suggestions

86a34df

ethanndickson previously approved these changes Jul 3, 2025

View reviewed changes

Fix unit test

42170ab

kacpersaw force-pushed the kacpersaw/cancel-pending-provisioner-jobs branch from 8081f33 to 42170ab Compare July 3, 2025 10:51

kacpersaw requested a review from BrunoQuaresma July 3, 2025 11:42

deansheather reviewed Jul 3, 2025

View reviewed changes

kacpersaw added 5 commits July 3, 2025 15:10

Apply review suggestions

5db9d71

Fix typo

1ede20c

Regenerate api types

2597615

Fix typo

6c2d0cf

Merge branch 'main' into kacpersaw/cancel-pending-provisioner-jobs

c800494

BrunoQuaresma reviewed Jul 3, 2025

View reviewed changes

Apply a new authorization check for GetProvisionerJobByIDForUpdate

c5cb203

kacpersaw force-pushed the kacpersaw/cancel-pending-provisioner-jobs branch from 11d8c0a to c5cb203 Compare July 6, 2025 09:48

kacpersaw added 2 commits July 6, 2025 10:06

Apply a new authorization check for GetProvisionerJobByIDForUpdate

1de84cc

Apply FE review suggestions

17fb6a3

kacpersaw requested a review from deansheather July 7, 2025 05:51

deansheather reviewed Jul 7, 2025

View reviewed changes

kacpersaw added 3 commits July 7, 2025 10:54

Apply review suggestions

1b7b614

Refactor cancelWorkspaceBuild parameter handling

634f556

Fix lint

4deace0

deansheather approved these changes Jul 7, 2025

View reviewed changes

kacpersaw and others added 2 commits July 7, 2025 14:30

Update coderd/workspacebuilds.go

43430fa

Co-authored-by: Dean Sheather <dean@deansheather.com>

Extract cancel confirm dialog to a separate component

6272d93

kacpersaw requested a review from BrunoQuaresma July 7, 2025 14:06

BrunoQuaresma approved these changes Jul 7, 2025

View reviewed changes

Merge branch 'main' into kacpersaw/cancel-pending-provisioner-jobs

4d4a01d

kacpersaw merged commit 8202514 into main Jul 8, 2025
63 of 64 checks passed

kacpersaw deleted the kacpersaw/cancel-pending-provisioner-jobs branch July 8, 2025 09:03

github-actions bot locked and limited conversation to collaborators Jul 8, 2025

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

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

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

	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("job is not in the expected state")
	return xerrors.Errorf("job is not in the expected state: expected %q, got %q", expectStatus, job.JobStatus)

		): Promise<TypesGen.Response> => {
		const params = request?.expect_status

	ExpectStatus CancelWorkspaceBuildStatus `json:"expect_status,omitempty"`
	ExpectStatus CancelWorkspaceBuildStatus `json:"-"`

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

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

Uh oh!

Conversation

kacpersaw commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethanndickson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deansheather left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deansheather left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deansheather left a comment

kacpersaw commented Jul 2, 2025 •

edited

Loading

BrunoQuaresma Jul 7, 2025 •

edited

Loading