From ba41ae807a0ad61aacf5362a129c3510c32bc07a Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Tue, 1 Jul 2025 12:31:46 +0000 Subject: [PATCH 1/5] add support for canceling workspace builds with expect_state param (locking) --- cli/provisionerjobs.go | 2 +- coderd/apidoc/docs.go | 6 ++ coderd/apidoc/swagger.json | 6 ++ coderd/workspacebuilds.go | 126 +++++++++++++++++-------- coderd/workspacebuilds_test.go | 156 ++++++++++++++++++++++++++++++- coderd/workspaces_test.go | 2 +- codersdk/toolsdk/toolsdk_test.go | 8 +- codersdk/workspacebuilds.go | 16 +++- docs/reference/api/builds.md | 7 +- scaletest/workspacebuild/run.go | 2 +- site/src/api/typesGenerated.ts | 5 + 11 files changed, 283 insertions(+), 53 deletions(-) diff --git a/cli/provisionerjobs.go b/cli/provisionerjobs.go index c2b6b78658447..efb4eec6e00e0 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.CancelWorkspaceBuildRequest{}) } if err != nil { return xerrors.Errorf("cancel provisioner job: %w", err) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 522ba671a9a63..c89703f0fbadf 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -8839,6 +8839,12 @@ const docTemplate = `{ "name": "workspacebuild", "in": "path", "required": true + }, + { + "type": "string", + "description": "Expected state of the job", + "name": "expect_state", + "in": "query" } ], "responses": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index abcae550a4ec5..1a681f66e094a 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -7824,6 +7824,12 @@ "name": "workspacebuild", "in": "path", "required": true + }, + { + "type": "string", + "description": "Expected state of the job", + "name": "expect_state", + "in": "query" } ], "responses": { diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 6c3321625c9b3..cb85e01a31f79 100644 --- a/coderd/workspacebuilds.go +++ b/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_state query string false "Expected state of the job" // @Success 200 {object} codersdk.Response // @Router /workspacebuilds/{workspacebuild}/cancel [patch] func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() + expectState := r.URL.Query().Get("expect_state") workspaceBuild := httpmw.WorkspaceBuildParam(r) workspace, err := api.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) if err != nil { @@ -609,46 +611,94 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques return } - 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 - } - 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 { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error updating provisioner job.", - Detail: err.Error(), + if expectState != "" { + jobStatus := database.ProvisionerJobStatus(expectState) + if !jobStatus.Valid() { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid expect_state. Only 'pending', 'running', 'succeeded', 'canceling', 'canceled', 'failed', or 'unknown' are allowed.", + }) + return + } + + // use local error type to detect if the error is due to job state mismatch + var errJobStateMismatch = xerrors.New("job is not in the expected state") + + err := api.Database.InTx(func(store database.Store) error { + job, err := store.GetProvisionerJobByIDForUpdate(ctx, workspaceBuild.JobID) + if err != nil { + return err + } + + if job.JobStatus != jobStatus { + return errJobStateMismatch + } + + return store.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{ + ID: job.ID, + CanceledAt: sql.NullTime{ + Time: dbtime.Now(), + Valid: true, + }, + CompletedAt: sql.NullTime{ + Time: dbtime.Now(), + Valid: !job.WorkerID.Valid, + }, + }) + }, nil) + if err != nil { + if errors.Is(err, errJobStateMismatch) { + httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{ + Message: "Job is not in the expected state.", + }) + return + } + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching provisioner job.", + Detail: err.Error(), + }) + return + } + } else { + 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 + } + 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, + }, }) - return + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error updating provisioner job.", + Detail: err.Error(), + }) + return + } } - api.publishWorkspaceUpdate(ctx, workspace.OwnerID, wspubsub.WorkspaceEvent{ Kind: wspubsub.WorkspaceEventKindStateChange, WorkspaceID: workspace.ID, diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index b9d32a00b139a..7b47560e74aca 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.CancelWorkspaceBuildRequest{}) require.NoError(t, err) require.Eventually(t, func() bool { var err error @@ -618,11 +618,161 @@ 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.CancelWorkspaceBuildRequest{}) 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.CancelWorkspaceBuildRequest{ + ExpectState: codersdk.ProvisionerJobPending, + }) + 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 - 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, api := coderdtest.NewWithAPI(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) + + // When: a provisioner daemon is started + daemon := coderdtest.NewProvisionerDaemon(t, api) + defer daemon.Close() + // Then: the job should be acquired (status changes from pending) + require.Eventually(t, func() bool { + build, err = client.WorkspaceBuild(ctx, build.ID) + return err == nil && build.Job.Status != codersdk.ProvisionerJobPending + }, testutil.WaitShort, testutil.IntervalFast) + + // When: a cancel request is made with expect_state=pending + err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{ + ExpectState: codersdk.ProvisionerJobPending, + }) + // Then: the request should fail with 412. + require.Error(t, err) + require.Equal(t, http.StatusPreconditionFailed, err.(*codersdk.Error).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, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // When: a cancel request is made with invalid expect_state + err := client.CancelWorkspaceBuild(ctx, workspace.LatestBuild.ID, codersdk.CancelWorkspaceBuildRequest{ + ExpectState: "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_state") + }) } func TestWorkspaceBuildResources(t *testing.T) { @@ -968,7 +1118,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.CancelWorkspaceBuildRequest{}) 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..b09f2f150d3f1 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.CancelWorkspaceBuildRequest{}) 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..645063894a07d 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.CancelWorkspaceBuildRequest{})) }) 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.CancelWorkspaceBuildRequest{})) }) 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.CancelWorkspaceBuildRequest{})) // 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.CancelWorkspaceBuildRequest{})) }) }) diff --git a/codersdk/workspacebuilds.go b/codersdk/workspacebuilds.go index 328b8bc26566f..d403336e804fc 100644 --- a/codersdk/workspacebuilds.go +++ b/codersdk/workspacebuilds.go @@ -123,9 +123,21 @@ func (c *Client) WorkspaceBuild(ctx context.Context, id uuid.UUID) (WorkspaceBui return workspaceBuild, json.NewDecoder(res.Body).Decode(&workspaceBuild) } +type CancelWorkspaceBuildRequest struct { + ExpectState ProvisionerJobStatus `json:"expect_state,omitempty"` +} + +func (c *CancelWorkspaceBuildRequest) asRequestOption() RequestOption { + return func(r *http.Request) { + q := r.URL.Query() + q.Set("expect_state", string(c.ExpectState)) + 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 CancelWorkspaceBuildRequest) 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..daf029a4ed203 100644 --- a/docs/reference/api/builds.md +++ b/docs/reference/api/builds.md @@ -491,9 +491,10 @@ 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_state` | query | string | false | Expected state of the job | ### Example responses diff --git a/scaletest/workspacebuild/run.go b/scaletest/workspacebuild/run.go index f19c556823faf..1645e371318d6 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.CancelWorkspaceBuildRequest{}); 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/typesGenerated.ts b/site/src/api/typesGenerated.ts index 0e6a481406d8b..e1311fd68af32 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -292,6 +292,11 @@ export const BypassRatelimitHeader = "X-Coder-Bypass-Ratelimit"; // From codersdk/client.go export const CLITelemetryHeader = "Coder-CLI-Telemetry"; +// From codersdk/workspacebuilds.go +export interface CancelWorkspaceBuildRequest { + readonly expect_state?: ProvisionerJobStatus; +} + // From codersdk/users.go export interface ChangePasswordWithOneTimePasscodeRequest { readonly email: string; From c4ee5b682a34255ef3003c493d5ce930a3d754b2 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Wed, 2 Jul 2025 10:24:48 +0000 Subject: [PATCH 2/5] Use single transaction for canceling workspace build --- coderd/apidoc/docs.go | 8 ++- coderd/apidoc/swagger.json | 5 +- coderd/workspacebuilds.go | 118 ++++++++++++++------------------- coderd/workspacebuilds_test.go | 8 +-- codersdk/workspacebuilds.go | 11 ++- docs/reference/api/builds.md | 15 +++-- 6 files changed, 82 insertions(+), 83 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index c89703f0fbadf..9423401da961d 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -8841,9 +8841,13 @@ const docTemplate = `{ "required": true }, { + "enum": [ + "running", + "pending" + ], "type": "string", - "description": "Expected state of the job", - "name": "expect_state", + "description": "Expected status of the job", + "name": "expect_status", "in": "query" } ], diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 1a681f66e094a..60ad635fa5062 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -7826,9 +7826,10 @@ "required": true }, { + "enum": ["running", "pending"], "type": "string", - "description": "Expected state of the job", - "name": "expect_state", + "description": "Expected status of the job", + "name": "expect_status", "in": "query" } ], diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index cb85e01a31f79..0b87e287c41f8 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -581,12 +581,12 @@ func (api *API) notifyWorkspaceUpdated( // @Produce json // @Tags Builds // @Param workspacebuild path string true "Workspace build ID" -// @Param expect_state query string false "Expected state of the job" +// @Param expect_status query string false "Expected status of the job" 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() - expectState := r.URL.Query().Get("expect_state") + expectStatus := r.URL.Query().Get("expect_status") workspaceBuild := httpmw.WorkspaceBuildParam(r) workspace, err := api.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) if err != nil { @@ -596,90 +596,60 @@ 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 - } - - if expectState != "" { - jobStatus := database.ProvisionerJobStatus(expectState) - if !jobStatus.Valid() { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Invalid expect_state. Only 'pending', 'running', 'succeeded', 'canceling', 'canceled', 'failed', or 'unknown' are allowed.", - }) - return - } - - // use local error type to detect if the error is due to job state mismatch - var errJobStateMismatch = xerrors.New("job is not in the expected state") - - err := api.Database.InTx(func(store database.Store) error { - job, err := store.GetProvisionerJobByIDForUpdate(ctx, workspaceBuild.JobID) - if err != nil { - return err - } - - if job.JobStatus != jobStatus { - return errJobStateMismatch - } - - return store.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{ - ID: job.ID, - CanceledAt: sql.NullTime{ - Time: dbtime.Now(), - Valid: true, - }, - CompletedAt: sql.NullTime{ - Time: dbtime.Now(), - Valid: !job.WorkerID.Valid, - }, - }) - }, nil) + err = api.Database.InTx(func(store database.Store) error { + valid, err := api.verifyUserCanCancelWorkspaceBuilds(ctx, store, httpmw.APIKey(r).UserID, workspace.TemplateID) if err != nil { - if errors.Is(err, errJobStateMismatch) { - httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{ - Message: "Job is not in the expected state.", - }) - return - } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching provisioner job.", + Message: "Internal error verifying permission to cancel workspace build.", Detail: err.Error(), }) - return + return nil + } + 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 } - } else { - job, err := api.Database.GetProvisionerJobByID(ctx, workspaceBuild.JobID) + + job, err := store.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 + return nil } if job.CompletedAt.Valid { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Job has already completed!", }) - return + return nil } if job.CanceledAt.Valid { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Job has already been marked as canceled!", }) - return + return nil + } + + 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 + } + + if job.JobStatus != database.ProvisionerJobStatus(expectStatus) { + httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{ + Message: "Job is not in the expected state.", + }) + return nil + } } - err = api.Database.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{ + + err = store.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{ ID: job.ID, CanceledAt: sql.NullTime{ Time: dbtime.Now(), @@ -696,9 +666,19 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques Message: "Internal error updating provisioner job.", Detail: err.Error(), }) - return + return nil } + + return nil + }, nil) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error updating provisioner job.", + Detail: err.Error(), + }) + return } + api.publishWorkspaceUpdate(ctx, workspace.OwnerID, wspubsub.WorkspaceEvent{ Kind: wspubsub.WorkspaceEventKindStateChange, WorkspaceID: workspace.ID, @@ -709,8 +689,8 @@ 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 (*API) 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") } @@ -719,7 +699,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 7b47560e74aca..3f94dbe3f2a40 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -672,7 +672,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { // When: the workspace build is canceled err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{ - ExpectState: codersdk.ProvisionerJobPending, + ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending, }) require.NoError(t, err) @@ -734,7 +734,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { // When: a cancel request is made with expect_state=pending err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{ - ExpectState: codersdk.ProvisionerJobPending, + ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending, }) // Then: the request should fail with 412. require.Error(t, err) @@ -765,13 +765,13 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { // When: a cancel request is made with invalid expect_state err := client.CancelWorkspaceBuild(ctx, workspace.LatestBuild.ID, codersdk.CancelWorkspaceBuildRequest{ - ExpectState: "invalid_status", + 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_state") + require.Contains(t, apiErr.Message, "Invalid expect_status") }) } diff --git a/codersdk/workspacebuilds.go b/codersdk/workspacebuilds.go index d403336e804fc..dcbd9a19831f8 100644 --- a/codersdk/workspacebuilds.go +++ b/codersdk/workspacebuilds.go @@ -123,14 +123,21 @@ 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 CancelWorkspaceBuildRequest struct { - ExpectState ProvisionerJobStatus `json:"expect_state,omitempty"` + ExpectStatus CancelWorkspaceBuildStatus `json:"expect_status,omitempty"` } func (c *CancelWorkspaceBuildRequest) asRequestOption() RequestOption { return func(r *http.Request) { q := r.URL.Query() - q.Set("expect_state", string(c.ExpectState)) + q.Set("expect_status", string(c.ExpectStatus)) r.URL.RawQuery = q.Encode() } } diff --git a/docs/reference/api/builds.md b/docs/reference/api/builds.md index daf029a4ed203..c3ea87d04adcf 100644 --- a/docs/reference/api/builds.md +++ b/docs/reference/api/builds.md @@ -491,10 +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 | -| `expect_state` | query | string | false | Expected state of the job | +| Name | In | Type | Required | Description | +|------------------|-------|--------|----------|----------------------------| +| `workspacebuild` | path | string | true | Workspace build ID | +| `expect_status` | query | string | false | Expected status of the job | + +#### Enumerated Values + +| Parameter | Value | +|-----------------|-----------| +| `expect_status` | `running` | +| `expect_status` | `pending` | ### Example responses From c49c33ea8d098286c1d3be48367e6d855e0bb06c Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Wed, 2 Jul 2025 10:26:13 +0000 Subject: [PATCH 3/5] Fix lint problem in ut --- coderd/workspacebuilds_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 3f94dbe3f2a40..9e78b3b5bb0c4 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -738,7 +738,10 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { }) // Then: the request should fail with 412. require.Error(t, err) - require.Equal(t, http.StatusPreconditionFailed, err.(*codersdk.Error).StatusCode()) + + 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) { From ba1dbf3268675fe8bed83487e8fdbb136ed7ce41 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Wed, 2 Jul 2025 12:34:04 +0000 Subject: [PATCH 4/5] add cancel confirmation dialog for workspace builds and add expect_status for pending builds --- site/src/api/api.ts | 7 ++- site/src/api/queries/workspaces.ts | 6 +++ site/src/api/typesGenerated.ts | 10 ++++- site/src/modules/workspaces/actions.ts | 2 +- .../WorkspacePage/WorkspacePage.test.tsx | 45 ++++++++++++++++++- .../WorkspacePage/WorkspaceReadyPage.tsx | 19 +++++++- .../pages/WorkspacesPage/WorkspacesTable.tsx | 19 +++++++- 7 files changed, 102 insertions(+), 6 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 458e93b32cdbe..4214f4d323b0b 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1277,9 +1277,14 @@ class ApiMethods { cancelWorkspaceBuild = async ( workspaceBuildId: TypesGen.WorkspaceBuild["id"], + request?: TypesGen.CancelWorkspaceBuildRequest, ): Promise => { + const params = request?.expect_status + ? new URLSearchParams({ expect_status: request.expect_status }).toString() + : ""; + const response = await this.axios.patch( - `/api/v2/workspacebuilds/${workspaceBuildId}/cancel`, + `/api/v2/workspacebuilds/${workspaceBuildId}/cancel${params ? `?${params}` : ""}`, ); return response.data; diff --git a/site/src/api/queries/workspaces.ts b/site/src/api/queries/workspaces.ts index 5a4cdb46dd4e9..414747af1367b 100644 --- a/site/src/api/queries/workspaces.ts +++ b/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 + if (workspace.latest_build.status === "pending") { + return API.cancelWorkspaceBuild(workspace.latest_build.id, { + expect_status: "pending", + }); + } return API.cancelWorkspaceBuild(workspace.latest_build.id); }, onSuccess: async () => { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index e1311fd68af32..547a0231b3d74 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -294,9 +294,17 @@ export const CLITelemetryHeader = "Coder-CLI-Telemetry"; // From codersdk/workspacebuilds.go export interface CancelWorkspaceBuildRequest { - readonly expect_state?: ProvisionerJobStatus; + 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/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 67a1a460dcd45..8d193b134eb80 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -18,6 +18,7 @@ import { MockFailedWorkspace, MockOrganization, MockOutdatedWorkspace, + MockPendingWorkspace, MockStartingWorkspace, MockStoppedWorkspace, MockTemplate, @@ -223,11 +224,53 @@ 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); + }); + + 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); - await testButton(MockStartingWorkspace, "Cancel", cancelWorkspaceMock); + 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 1e727faf46cd4..67d082526bf79 100644 --- a/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx @@ -80,6 +80,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 +318,7 @@ export const WorkspaceReadyPage: FC = ({ } }} handleUpdate={workspaceUpdate.update} - handleCancel={cancelBuildMutation.mutate} + handleCancel={() => setIsCancelConfirmOpen(true)} handleRetry={handleRetry} handleDebug={handleDebug} handleDormantActivate={async () => { @@ -352,6 +354,21 @@ export const WorkspaceReadyPage: FC = ({ } /> + {/* Cancel confirmation dialog */} + setIsCancelConfirmOpen(false)} + onConfirm={() => { + cancelBuildMutation.mutate(); + setIsCancelConfirmOpen(false); + }} + type="delete" + /> + diff --git a/site/src/pages/WorkspacesPage/WorkspacesTable.tsx b/site/src/pages/WorkspacesPage/WorkspacesTable.tsx index 6213224dea602..03b7b4692f9c3 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesTable.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesTable.tsx @@ -497,6 +497,8 @@ const WorkspaceActionsCell: FC = ({ // State for stop confirmation dialog const [isStopConfirmOpen, setIsStopConfirmOpen] = useState(false); + // State for cancel confirmation dialog + const [isCancelConfirmOpen, setIsCancelConfirmOpen] = useState(false); const isRetrying = startWorkspaceMutation.isPending || @@ -606,7 +608,7 @@ const WorkspaceActionsCell: FC = ({ {abilities.canCancel && ( setIsCancelConfirmOpen(true)} isLoading={cancelBuildMutation.isPending} label="Cancel build" > @@ -643,6 +645,21 @@ const WorkspaceActionsCell: FC = ({ }} type="delete" /> + + {/* Cancel workspace build confirmation dialog */} + setIsCancelConfirmOpen(false)} + onConfirm={() => { + cancelBuildMutation.mutate(); + setIsCancelConfirmOpen(false); + }} + type="delete" + /> ); }; From acffda65b9c318e808fc022130baea22a8d72d9b Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Wed, 2 Jul 2025 12:55:06 +0000 Subject: [PATCH 5/5] Fix lint --- site/src/pages/WorkspacePage/WorkspacePage.test.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index 8d193b134eb80..9581244e603fa 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -242,7 +242,9 @@ describe("WorkspacePage", () => { }); await user.click(confirmButton); - expect(cancelWorkspaceMock).toHaveBeenCalledWith(MockStartingWorkspace.latest_build.id); + expect(cancelWorkspaceMock).toHaveBeenCalledWith( + MockStartingWorkspace.latest_build.id, + ); }); it("requests cancellation when the user presses Cancel and the workspace is pending", async () => { @@ -270,7 +272,10 @@ describe("WorkspacePage", () => { }); await user.click(confirmButton); - expect(cancelWorkspaceMock).toHaveBeenCalledWith(MockPendingWorkspace.latest_build.id, { expect_status: "pending" }); + expect(cancelWorkspaceMock).toHaveBeenCalledWith( + MockPendingWorkspace.latest_build.id, + { expect_status: "pending" }, + ); }); it("requests an update when the user presses Update", async () => {