Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

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

Already on GitHub?Sign in to your account

Merged
kacpersaw merged 22 commits intomainfromkacpersaw/cancel-pending-provisioner-jobs
Jul 8, 2025

Conversation

kacpersaw
Copy link
Contributor

@kacpersawkacpersaw commentedJul 2, 2025•
edited
Loading

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:

  • Addedexpect_status query parameter to the cancel workspace build endpoint
  • This parameter ensures the job hasn't changed state before canceling
  • API returns412 Precondition Failed if the job is not in the expected status
  • Valid values:running orpending
  • Wrapped the entire cancel method in a database transaction

UI:

  • Added confirmation dialog to theCancel button, since it's a destructive operation
    image
    image

  • Enabled cancel action for pending workspaces (expect_status=pending is sent if workspace is in pending status)
    image

@kacpersawkacpersaw marked this pull request as ready for reviewJuly 2, 2025 13:52
@github-actionsgithub-actionsbot added the release/breakingThis label is applied to PRs to detect breaking changes as part of the release process labelJul 2, 2025
Copy link
Member

@ethanndicksonethanndickson left a comment

Choose a reason for hiding this comment

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

Looking good, just one comment w/ the transaction

ethanndickson
ethanndickson previously approved these changesJul 3, 2025
@kacpersawkacpersawforce-pushed thekacpersaw/cancel-pending-provisioner-jobs branch from8081f33 to42170abCompareJuly 3, 2025 10:51
Copy link
Member

@deansheatherdeansheather left a comment

Choose a reason for hiding this comment

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

BE only review

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)
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 namedb in these transaction closures

kacpersaw reacted with thumbs up emoji
Comment on lines 610 to 615
if!valid {
code=http.StatusForbidden
resp.Message="User is not allowed to cancel workspace builds. Owner role is required."

returnxerrors.Errorf("user is not allowed to cancel workspace builds")
}
Copy link
Member

Choose a reason for hiding this comment

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

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

kacpersaw reacted with thumbs up emoji
returnxerrors.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 aSELECT ... FOR UPDATE query instead, otherwise there's nothing preventing this job from being picked up during the cancel operation.

Copy link
ContributorAuthor

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 🙅

code=http.StatusBadRequest
resp.Message="Job has already completed!"

returnxerrors.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
returnxerrors.Errorf("job has already completed")
returnxerrors.New("job has already completed")

kacpersaw reacted with thumbs up emoji
code=http.StatusBadRequest
resp.Message="Job has already been marked as canceled!"

returnxerrors.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
returnxerrors.Errorf("job has already been marked as canceled")
returnxerrors.New("job has already been marked as canceled")

kacpersaw reacted with thumbs up emoji
code=http.StatusBadRequest
resp.Message="Invalid expect_status. Only 'running' or 'pending' are allowed."

returnxerrors.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
returnxerrors.Errorf("invalid expect_status")
returnxerrors.Errorf("invalid expect_status %q",expectStatus)

kacpersaw reacted with thumbs up emoji
ifexpectStatus!="" {
ifexpectStatus!="running"&&expectStatus!="pending" {
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)

kacpersaw reacted with thumbs up emoji
code=http.StatusPreconditionFailed
resp.Message="Job is not in the expected state."

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

kacpersaw reacted with thumbs up emoji
CancelWorkspaceBuildStatusPendingCancelWorkspaceBuildStatus="pending"
)

typeCancelWorkspaceBuildRequeststruct {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call thisParams instead ofRequest since it's not the request body

kacpersaw reacted with thumbs up emoji
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

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

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.

kacpersaw reacted with thumbs up emoji

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

Choose a reason for hiding this comment

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

Since the type has the "Params" sufix, I would name this variable asparams instead ofrequest.

workspaceBuildId:TypesGen.WorkspaceBuild["id"],
request?:TypesGen.CancelWorkspaceBuildParams,
):Promise<TypesGen.Response>=>{
constparams=request?.expect_status
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can do:

constparams=newURLSearchParams(params)


constresponse=awaitthis.axios.patch(
`/api/v2/workspacebuilds/${workspaceBuildId}/cancel`,
`/api/v2/workspacebuilds/${workspaceBuildId}/cancel${params ?`?${params}` :""}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

And just use it directly:

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

exportconstcancelBuild=(workspace:Workspace,queryClient:QueryClient)=>{
return{
mutationFn:()=>{
// If workspace status is pending, include expect_status parameter
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 theexpect_status to pending.

}
/>

{/* Cancel confirmation dialog */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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


// From codersdk/workspacebuilds.go
exportinterfaceCancelWorkspaceBuildParams{
readonlyexpect_status?:CancelWorkspaceBuildStatus;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 theCancelWorkspaceBuildParams.ExpectStatus could be helpful to understand when and why use it.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

BrunoQuaresma reacted with confused emoji
@kacpersawkacpersawforce-pushed thekacpersaw/cancel-pending-provisioner-jobs branch from11d8c0a toc5cb203CompareJuly 6, 2025 09:48
Copy link
Member

@deansheatherdeansheather left a comment

Choose a reason for hiding this comment

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

BG looks good, just some minor comments

// @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)
Copy link
Member

Choose a reason for hiding this comment

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

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:

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

Comment on lines 639 to 644
ifexpectStatus!="running"&&expectStatus!="pending" {
code=http.StatusBadRequest
resp.Message=fmt.Sprintf("Invalid expect_status %q. Only 'running' or 'pending' are allowed.",expectStatus)

returnxerrors.Errorf("invalid expect_status %q",expectStatus)
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

})
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.


func (api*API)verifyUserCanCancelWorkspaceBuilds(ctx context.Context,userID uuid.UUID,templateID uuid.UUID) (bool,error) {
template,err:=api.Database.GetTemplateByID(ctx,templateID)
funcverifyUserCanCancelWorkspaceBuilds(ctx context.Context,store database.Store,userID uuid.UUID,templateID uuid.UUID,expectStatusstring) (bool,error) {
Copy link
Member

Choose a reason for hiding this comment

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

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

returnassert.NoError(t,err)&&build.Job.Status==codersdk.ProvisionerJobRunning
},testutil.WaitShort,testutil.IntervalFast)

// When: a cancel request is made with expect_state=pending
Copy link
Member

Choose a reason for hiding this comment

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

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 theexpect_status param

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

deansheather reacted with thumbs up emoji

typeCancelWorkspaceBuildParamsstruct {
// ExpectStatus ensures the build is in the expected status before canceling.
ExpectStatusCancelWorkspaceBuildStatus`json:"expect_status,omitempty"`
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
ExpectStatusCancelWorkspaceBuildStatus`json:"expect_status,omitempty"`
ExpectStatusCancelWorkspaceBuildStatus`json:"-"`

Since it's never used as JSON

Copy link
Member

Choose a reason for hiding this comment

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

Or just omit thejson tag entirely perhaps

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

deansheather reacted with thumbs up emoji
Copy link
Member

@deansheatherdeansheather left a comment

Choose a reason for hiding this comment

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

Backend looks great!

<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.`}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

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

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

status==="pending"||status==="running"
?{expect_status:status}
:undefined;
returnAPI.cancelWorkspaceBuild(workspace.latest_build.id,params);
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresmaJul 7, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Double checking... cancancelWorkspaceBuild being called in any status? I see the params can beundefined, but it would still call thecancelWorkspaceBuild 🤔. 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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

@kacpersawkacpersaw merged commit8202514 intomainJul 8, 2025
58 of 59 checks passed
@kacpersawkacpersaw deleted the kacpersaw/cancel-pending-provisioner-jobs branchJuly 8, 2025 09:03
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJul 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma approved these changes

@deansheatherdeansheatherdeansheather approved these changes

@ethanndicksonethanndicksonethanndickson left review comments

Assignees

@kacpersawkacpersaw

Labels
release/breakingThis label is applied to PRs to detect breaking changes as part of the release process
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to cancel Pending provisioner jobs in the UI
4 participants
@kacpersaw@BrunoQuaresma@deansheather@ethanndickson
[8]ページ先頭
©2009-2025 Movatter.jp