From ba41ae807a0ad61aacf5362a129c3510c32bc07a Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Tue, 1 Jul 2025 12:31:46 +0000 Subject: [PATCH 01/19] 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 02/19] 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 03/19] 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 04/19] 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 05/19] 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 () => { From 86a34dfba1557356fee2509a30555cf495ecc221 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 3 Jul 2025 09:52:55 +0000 Subject: [PATCH 06/19] Apply review suggestions --- coderd/workspacebuilds.go | 81 +++++++++++++++++----------------- coderd/workspacebuilds_test.go | 3 +- 2 files changed, 41 insertions(+), 43 deletions(-) diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 0b87e287c41f8..b7c53a374fd30 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -596,56 +596,58 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques return } + code := http.StatusOK + 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) 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) 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") } 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") } 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." + + return xerrors.Errorf("invalid expect_status") } 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") } } @@ -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 } @@ -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") diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 9e78b3b5bb0c4..d53ba497a44b7 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -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{ From 42170ab888b48e0af250b3067b1919b9d731d01a Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 3 Jul 2025 10:40:54 +0000 Subject: [PATCH 07/19] Fix unit test --- coderd/workspacebuilds_test.go | 56 +++++++++++----------------------- 1 file changed, 17 insertions(+), 39 deletions(-) diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index d53ba497a44b7..45ba0722f046c 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -684,56 +684,34 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { 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 + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) user := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + 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) - 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) + var build codersdk.WorkspaceBuild - // 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) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() - // 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 + var err error + build, err = client.WorkspaceBuild(ctx, workspace.LatestBuild.ID) + return assert.NoError(t, err) && build.Job.Status == codersdk.ProvisionerJobRunning }, testutil.WaitShort, testutil.IntervalFast) // When: a cancel request is made with expect_state=pending - err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{ + err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{ ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending, }) // Then: the request should fail with 412. From 5db9d716df21f6c97670a17fd4fd1fca7e3cd75f Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 3 Jul 2025 15:08:46 +0000 Subject: [PATCH 08/19] Apply review suggestions --- coderd/workspacebuilds.go | 27 ++++++++++++++++----------- coderd/workspacebuilds_test.go | 12 ++++++------ coderd/workspaces_test.go | 2 +- codersdk/toolsdk/toolsdk_test.go | 8 ++++---- codersdk/workspacebuilds.go | 6 +++--- scaletest/workspacebuild/run.go | 2 +- 6 files changed, 31 insertions(+), 26 deletions(-) diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index b7c53a374fd30..fb04c0594e2dd 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -598,8 +598,8 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques 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) + err = api.Database.InTx(func(db database.Store) error { + valid, err := verifyUserCanCancelWorkspaceBuilds(ctx, db, httpmw.APIKey(r).UserID, workspace.TemplateID, expectStatus) if err != nil { code = http.StatusInternalServerError resp.Message = "Internal error verifying permission to cancel workspace build." @@ -611,10 +611,10 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques 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") + return xerrors.New("user is not allowed to cancel workspace builds") } - job, err := store.GetProvisionerJobByID(ctx, workspaceBuild.JobID) + job, err := db.GetProvisionerJobByIDForUpdate(ctx, workspaceBuild.JobID) if err != nil { code = http.StatusInternalServerError resp.Message = "Internal error fetching provisioner job." @@ -626,32 +626,32 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques code = http.StatusBadRequest resp.Message = "Job has already completed!" - return xerrors.Errorf("job has already completed") + return xerrors.New("job has already completed") } if job.CanceledAt.Valid { code = http.StatusBadRequest resp.Message = "Job has already been marked as canceled!" - 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" { code = http.StatusBadRequest - 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") + return xerrors.Errorf("invalid expect_status %q", expectStatus) } if job.JobStatus != database.ProvisionerJobStatus(expectStatus) { code = http.StatusPreconditionFailed resp.Message = "Job is not in the expected state." - 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) } } - err = store.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{ + err = db.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{ ID: job.ID, CanceledAt: sql.NullTime{ Time: dbtime.Now(), @@ -688,7 +688,12 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques }) } -func 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, expectStatus string) (bool, error) { + // If the expectStatus is pending, we can cancel it. + if expectStatus == "pending" { + return true, nil + } + template, err := store.GetTemplateByID(ctx, templateID) if err != nil { return false, xerrors.New("no template exists for this workspace") diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 45ba0722f046c..c4bfb93dbdc33 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, codersdk.CancelWorkspaceBuildRequest{}) + err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}) require.NoError(t, err) require.Eventually(t, func() bool { var err error @@ -618,7 +618,7 @@ 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, codersdk.CancelWorkspaceBuildRequest{}) + err := userClient.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) @@ -671,7 +671,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { } // When: the workspace build is canceled - err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{ + err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{ ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending, }) require.NoError(t, err) @@ -711,7 +711,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { }, testutil.WaitShort, testutil.IntervalFast) // When: a cancel request is made with expect_state=pending - err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{ + err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{ ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending, }) // Then: the request should fail with 412. @@ -744,7 +744,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { 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{ + err := client.CancelWorkspaceBuild(ctx, workspace.LatestBuild.ID, codersdk.CancelWorkspaceBuildParams{ ExpectStatus: "invalid_status", }) // Then: the request should fail with 400. @@ -1098,7 +1098,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, codersdk.CancelWorkspaceBuildRequest{}) + err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}) require.NoError(t, err) workspace, err = client.Workspace(ctx, workspace.ID) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index b09f2f150d3f1..3a50f850da5e7 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, codersdk.CancelWorkspaceBuildRequest{}) + err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}) 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 645063894a07d..09b919a428a84 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, codersdk.CancelWorkspaceBuildRequest{})) + require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID, codersdk.CancelWorkspaceBuildParams{})) }) 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, codersdk.CancelWorkspaceBuildRequest{})) + require.NoError(t, client.CancelWorkspaceBuild(ctx, result.ID, codersdk.CancelWorkspaceBuildParams{})) }) 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, codersdk.CancelWorkspaceBuildRequest{})) + require.NoError(t, client.CancelWorkspaceBuild(ctx, updateBuild.ID, codersdk.CancelWorkspaceBuildParams{})) // 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, codersdk.CancelWorkspaceBuildRequest{})) + require.NoError(t, client.CancelWorkspaceBuild(ctx, rollbackBuild.ID, codersdk.CancelWorkspaceBuildParams{})) }) }) diff --git a/codersdk/workspacebuilds.go b/codersdk/workspacebuilds.go index dcbd9a19831f8..4898aab59ae5e 100644 --- a/codersdk/workspacebuilds.go +++ b/codersdk/workspacebuilds.go @@ -130,11 +130,11 @@ const ( CancelWorkspaceBuildStatusPending CancelWorkspaceBuildStatus = "pending" ) -type CancelWorkspaceBuildRequest struct { +type CancelWorkspaceBuildParams struct { ExpectStatus CancelWorkspaceBuildStatus `json:"expect_status,omitempty"` } -func (c *CancelWorkspaceBuildRequest) asRequestOption() RequestOption { +func (c *CancelWorkspaceBuildParams) asRequestOption() RequestOption { return func(r *http.Request) { q := r.URL.Query() q.Set("expect_status", string(c.ExpectStatus)) @@ -143,7 +143,7 @@ func (c *CancelWorkspaceBuildRequest) asRequestOption() RequestOption { } // CancelWorkspaceBuild marks a workspace build job as canceled. -func (c *Client) CancelWorkspaceBuild(ctx context.Context, id uuid.UUID, req CancelWorkspaceBuildRequest) error { +func (c *Client) CancelWorkspaceBuild(ctx context.Context, id uuid.UUID, req CancelWorkspaceBuildParams) 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/scaletest/workspacebuild/run.go b/scaletest/workspacebuild/run.go index 1645e371318d6..3b369a4e48a72 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, codersdk.CancelWorkspaceBuildRequest{}); err == nil { + if err = r.client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{}); 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 { From 1ede20c08e063afa1bf1ce76bc095da6b1c07d22 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 3 Jul 2025 15:27:01 +0000 Subject: [PATCH 09/19] Fix typo --- cli/provisionerjobs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/provisionerjobs.go b/cli/provisionerjobs.go index efb4eec6e00e0..2ddd04c5b6a29 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), codersdk.CancelWorkspaceBuildRequest{}) + err = client.CancelWorkspaceBuild(ctx, ptr.NilToEmpty(job.Input.WorkspaceBuildID), codersdk.CancelWorkspaceBuildParams{}) } if err != nil { return xerrors.Errorf("cancel provisioner job: %w", err) From 2597615b5f3b394218abf58cc961be9f439b55e4 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 3 Jul 2025 15:32:38 +0000 Subject: [PATCH 10/19] Regenerate api types --- site/src/api/typesGenerated.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 386832e38a0d5..73a164c73e108 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -293,7 +293,7 @@ export const BypassRatelimitHeader = "X-Coder-Bypass-Ratelimit"; export const CLITelemetryHeader = "Coder-CLI-Telemetry"; // From codersdk/workspacebuilds.go -export interface CancelWorkspaceBuildRequest { +export interface CancelWorkspaceBuildParams { readonly expect_status?: CancelWorkspaceBuildStatus; } From 6c2d0cfb32f72313542923f19b13afae31ee4b7f Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Thu, 3 Jul 2025 15:39:43 +0000 Subject: [PATCH 11/19] Fix typo --- site/src/api/api.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 4214f4d323b0b..bb9c596a7e712 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1277,7 +1277,7 @@ class ApiMethods { cancelWorkspaceBuild = async ( workspaceBuildId: TypesGen.WorkspaceBuild["id"], - request?: TypesGen.CancelWorkspaceBuildRequest, + request?: TypesGen.CancelWorkspaceBuildParams, ): Promise => { const params = request?.expect_status ? new URLSearchParams({ expect_status: request.expect_status }).toString() From c5cb2034b60f04c5fafa59b2f93e253675770913 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Sun, 6 Jul 2025 09:27:19 +0000 Subject: [PATCH 12/19] Apply a new authorization check for GetProvisionerJobByIDForUpdate --- coderd/database/dbauthz/dbauthz.go | 49 +++++++++++++++++++----------- codersdk/workspacebuilds.go | 1 + 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index eea1b04a51fc5..b4162f0db59d7 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1182,6 +1182,27 @@ func (q *querier) customRoleCheck(ctx context.Context, role database.CustomRole) return nil } +func (q *querier) authorizeProvisionerJob(ctx context.Context, job database.ProvisionerJob) error { + switch job.Type { + case database.ProvisionerJobTypeWorkspaceBuild: + // Authorized call to get workspace build. If we can read the build, we + // can read the job. + _, err := q.GetWorkspaceBuildByJobID(ctx, job.ID) + if err != nil { + return xerrors.Errorf("fetch related workspace build: %w", err) + } + case database.ProvisionerJobTypeTemplateVersionDryRun, database.ProvisionerJobTypeTemplateVersionImport: + // Authorized call to get template version. + _, err := authorizedTemplateVersionFromJob(ctx, q, job) + if err != nil { + return xerrors.Errorf("fetch related template version: %w", err) + } + default: + return xerrors.Errorf("unknown job type: %q", job.Type) + } + return nil +} + func (q *querier) AcquireLock(ctx context.Context, id int64) error { return q.db.AcquireLock(ctx, id) } @@ -2445,32 +2466,24 @@ func (q *querier) GetProvisionerJobByID(ctx context.Context, id uuid.UUID) (data return database.ProvisionerJob{}, err } - switch job.Type { - case database.ProvisionerJobTypeWorkspaceBuild: - // Authorized call to get workspace build. If we can read the build, we - // can read the job. - _, err := q.GetWorkspaceBuildByJobID(ctx, id) - if err != nil { - return database.ProvisionerJob{}, xerrors.Errorf("fetch related workspace build: %w", err) - } - case database.ProvisionerJobTypeTemplateVersionDryRun, database.ProvisionerJobTypeTemplateVersionImport: - // Authorized call to get template version. - _, err := authorizedTemplateVersionFromJob(ctx, q, job) - if err != nil { - return database.ProvisionerJob{}, xerrors.Errorf("fetch related template version: %w", err) - } - default: - return database.ProvisionerJob{}, xerrors.Errorf("unknown job type: %q", job.Type) + if err := q.authorizeProvisionerJob(ctx, job); err != nil { + return database.ProvisionerJob{}, err } return job, nil } func (q *querier) GetProvisionerJobByIDForUpdate(ctx context.Context, id uuid.UUID) (database.ProvisionerJob, error) { - if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil { + job, err := q.db.GetProvisionerJobByIDForUpdate(ctx, id) + if err != nil { return database.ProvisionerJob{}, err } - return q.db.GetProvisionerJobByIDForUpdate(ctx, id) + + if err := q.authorizeProvisionerJob(ctx, job); err != nil { + return database.ProvisionerJob{}, err + } + + return job, nil } func (q *querier) GetProvisionerJobTimingsByJobID(ctx context.Context, jobID uuid.UUID) ([]database.ProvisionerJobTiming, error) { diff --git a/codersdk/workspacebuilds.go b/codersdk/workspacebuilds.go index 4898aab59ae5e..4066341773994 100644 --- a/codersdk/workspacebuilds.go +++ b/codersdk/workspacebuilds.go @@ -131,6 +131,7 @@ const ( ) type CancelWorkspaceBuildParams struct { + // ExpectStatus ensures the build is in the expected status before canceling. ExpectStatus CancelWorkspaceBuildStatus `json:"expect_status,omitempty"` } From 1de84cc92245e1729d35bf447409dd56f43f2631 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Sun, 6 Jul 2025 09:27:19 +0000 Subject: [PATCH 13/19] Apply a new authorization check for GetProvisionerJobByIDForUpdate --- coderd/database/dbauthz/dbauthz_test.go | 55 ++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 006320ef459a4..bcbe6bb881dde 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -4655,8 +4655,59 @@ func (s *MethodTestSuite) TestSystemFunctions() { VapidPrivateKey: "test", }).Asserts(rbac.ResourceDeploymentConfig, policy.ActionUpdate) })) - s.Run("GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) { - check.Args(uuid.New()).Asserts(rbac.ResourceProvisionerJobs, policy.ActionRead).Errors(sql.ErrNoRows) + s.Run("Build/GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) { + u := dbgen.User(s.T(), db, database.User{}) + o := dbgen.Organization(s.T(), db, database.Organization{}) + tpl := dbgen.Template(s.T(), db, database.Template{ + OrganizationID: o.ID, + CreatedBy: u.ID, + }) + w := dbgen.Workspace(s.T(), db, database.WorkspaceTable{ + OwnerID: u.ID, + OrganizationID: o.ID, + TemplateID: tpl.ID, + }) + j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{ + Type: database.ProvisionerJobTypeWorkspaceBuild, + }) + tv := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true}, + JobID: j.ID, + OrganizationID: o.ID, + CreatedBy: u.ID, + }) + _ = dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{ + JobID: j.ID, + WorkspaceID: w.ID, + TemplateVersionID: tv.ID, + }) + check.Args(j.ID).Asserts(w, policy.ActionRead).Returns(j) + })) + s.Run("TemplateVersion/GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) { + dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) + j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{ + Type: database.ProvisionerJobTypeTemplateVersionImport, + }) + tpl := dbgen.Template(s.T(), db, database.Template{}) + v := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true}, + JobID: j.ID, + }) + check.Args(j.ID).Asserts(v.RBACObject(tpl), policy.ActionRead).Returns(j) + })) + s.Run("TemplateVersionDryRun/GetProvisionerJobByIDForUpdate", s.Subtest(func(db database.Store, check *expects) { + dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) + tpl := dbgen.Template(s.T(), db, database.Template{}) + v := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true}, + }) + j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{ + Type: database.ProvisionerJobTypeTemplateVersionDryRun, + Input: must(json.Marshal(struct { + TemplateVersionID uuid.UUID `json:"template_version_id"` + }{TemplateVersionID: v.ID})), + }) + check.Args(j.ID).Asserts(v.RBACObject(tpl), policy.ActionRead).Returns(j) })) s.Run("HasTemplateVersionsWithAITask", s.Subtest(func(db database.Store, check *expects) { check.Args().Asserts() From 17fb6a3b1a5b156adb379f1cf0eeb0f380adb411 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Sun, 6 Jul 2025 10:28:19 +0000 Subject: [PATCH 14/19] Apply FE review suggestions --- site/src/api/api.ts | 9 +++------ site/src/api/queries/workspaces.ts | 1 - site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx | 1 - site/src/pages/WorkspacesPage/WorkspacesTable.tsx | 2 -- 4 files changed, 3 insertions(+), 10 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 8a74ccfb16f8a..6c178cc5ed77a 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1277,14 +1277,11 @@ class ApiMethods { cancelWorkspaceBuild = async ( workspaceBuildId: TypesGen.WorkspaceBuild["id"], - request?: TypesGen.CancelWorkspaceBuildParams, + params?: TypesGen.CancelWorkspaceBuildParams, ): Promise => { - const params = request?.expect_status - ? new URLSearchParams({ expect_status: request.expect_status }).toString() - : ""; - + const queryParams = new URLSearchParams({ ...params }); const response = await this.axios.patch( - `/api/v2/workspacebuilds/${workspaceBuildId}/cancel${params ? `?${params}` : ""}`, + `/api/v2/workspacebuilds/${workspaceBuildId}/cancel${queryParams}`, ); return response.data; diff --git a/site/src/api/queries/workspaces.ts b/site/src/api/queries/workspaces.ts index 414747af1367b..cf52142677a63 100644 --- a/site/src/api/queries/workspaces.ts +++ b/site/src/api/queries/workspaces.ts @@ -266,7 +266,6 @@ 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", diff --git a/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx b/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx index f80b0fef80bb1..c46d73376ca5b 100644 --- a/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx @@ -354,7 +354,6 @@ export const WorkspaceReadyPage: FC = ({ } /> - {/* Cancel confirmation dialog */} = ({ onError: onActionError, }); - // State for stop confirmation dialog const [isStopConfirmOpen, setIsStopConfirmOpen] = useState(false); - // State for cancel confirmation dialog const [isCancelConfirmOpen, setIsCancelConfirmOpen] = useState(false); const isRetrying = From 1b7b614da1f82ea2b29b688c1d099075b5a5de32 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Mon, 7 Jul 2025 10:54:47 +0000 Subject: [PATCH 15/19] Apply review suggestions --- coderd/apidoc/docs.go | 2 +- coderd/apidoc/swagger.json | 2 +- coderd/workspacebuilds.go | 45 ++++++++++++---------- coderd/workspacebuilds_test.go | 62 +++++++++++++++++++++++++++++- docs/reference/api/builds.md | 8 ++-- site/src/api/api.ts | 5 ++- site/src/api/queries/workspaces.ts | 13 ++++--- 7 files changed, 101 insertions(+), 36 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index addbc7fd961e2..00c0db024e548 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -9132,7 +9132,7 @@ const docTemplate = `{ "pending" ], "type": "string", - "description": "Expected status of the job", + "description": "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.", "name": "expect_status", "in": "query" } diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index f26670a547d66..485215fe5f574 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -8072,7 +8072,7 @@ { "enum": ["running", "pending"], "type": "string", - "description": "Expected status of the job", + "description": "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.", "name": "expect_status", "in": "query" } diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index fb04c0594e2dd..dc58c75033f2a 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -581,12 +581,24 @@ 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) +// @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) // @Success 200 {object} codersdk.Response // @Router /workspacebuilds/{workspacebuild}/cancel [patch] func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - expectStatus := r.URL.Query().Get("expect_status") + + var expectStatus database.ProvisionerJobStatus + expectStatusParam := r.URL.Query().Get("expect_status") + if expectStatusParam != "" { + if expectStatusParam != "running" && expectStatusParam != "pending" { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Invalid expect_status %q. Only 'running' or 'pending' are allowed.", expectStatusParam), + }) + return + } + expectStatus = database.ProvisionerJobStatus(expectStatusParam) + } + workspaceBuild := httpmw.WorkspaceBuildParam(r) workspace, err := api.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) if err != nil { @@ -596,8 +608,10 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques return } - code := http.StatusOK - resp := codersdk.Response{} + code := http.StatusInternalServerError + resp := codersdk.Response{ + Message: "Internal error canceling workspace build.", + } err = api.Database.InTx(func(db database.Store) error { valid, err := verifyUserCanCancelWorkspaceBuilds(ctx, db, httpmw.APIKey(r).UserID, workspace.TemplateID, expectStatus) if err != nil { @@ -635,20 +649,11 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques return xerrors.New("job has already been marked as canceled") } - if expectStatus != "" { - 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) - } + if expectStatus != "" && job.JobStatus != expectStatus { + code = http.StatusPreconditionFailed + resp.Message = "Job is not in the expected state." - if job.JobStatus != database.ProvisionerJobStatus(expectStatus) { - code = http.StatusPreconditionFailed - resp.Message = "Job is not in the expected state." - - return xerrors.Errorf("job is not in the expected state: expected: %q, got %q", expectStatus, job.JobStatus) - } + return xerrors.Errorf("job is not in the expected state: expected: %q, got %q", expectStatus, job.JobStatus) } err = db.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{ @@ -688,9 +693,9 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques }) } -func verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID, expectStatus string) (bool, error) { - // If the expectStatus is pending, we can cancel it. - if expectStatus == "pending" { +func verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID, jobStatus database.ProvisionerJobStatus) (bool, error) { + // If the jobStatus is pending, we can cancel it. + if jobStatus == database.ProvisionerJobStatusPending { return true, nil } diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index c4bfb93dbdc33..ebab0770b71b4 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -682,7 +682,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { require.Equal(t, codersdk.ProvisionerJobCanceled, build.Job.Status) }) - t.Run("Cancel with expect_state=pending - should fail with 412", func(t *testing.T) { + t.Run("Cancel with expect_state=pending when job is running - should fail with 412", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) @@ -699,11 +699,11 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) workspace := coderdtest.CreateWorkspace(t, client, template.ID) - var build codersdk.WorkspaceBuild ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() + var build codersdk.WorkspaceBuild require.Eventually(t, func() bool { var err error build, err = client.WorkspaceBuild(ctx, workspace.LatestBuild.ID) @@ -722,6 +722,64 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { require.Equal(t, http.StatusPreconditionFailed, apiErr.StatusCode()) }) + t.Run("Cancel with expect_state=running when job is 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 := 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: a cancel request is made with expect_state=running + err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{ + ExpectStatus: codersdk.CancelWorkspaceBuildStatusRunning, + }) + // Then: the request should fail with 412. + require.Error(t, err) + + 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) { t.Parallel() diff --git a/docs/reference/api/builds.md b/docs/reference/api/builds.md index c3ea87d04adcf..686f19316a8c0 100644 --- a/docs/reference/api/builds.md +++ b/docs/reference/api/builds.md @@ -491,10 +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 | -| `expect_status` | query | string | false | Expected status 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. 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. | #### Enumerated Values diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 6c178cc5ed77a..dd8d3d77998d2 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1279,9 +1279,10 @@ class ApiMethods { workspaceBuildId: TypesGen.WorkspaceBuild["id"], params?: TypesGen.CancelWorkspaceBuildParams, ): Promise => { - const queryParams = new URLSearchParams({ ...params }); const response = await this.axios.patch( - `/api/v2/workspacebuilds/${workspaceBuildId}/cancel${queryParams}`, + `/api/v2/workspacebuilds/${workspaceBuildId}/cancel`, + null, + { params }, ); return response.data; diff --git a/site/src/api/queries/workspaces.ts b/site/src/api/queries/workspaces.ts index cf52142677a63..a8f9b0e8f49ee 100644 --- a/site/src/api/queries/workspaces.ts +++ b/site/src/api/queries/workspaces.ts @@ -1,6 +1,7 @@ import { API, type DeleteWorkspaceOptions } from "api/api"; import { DetailedError, isApiValidationError } from "api/errors"; import type { + CancelWorkspaceBuildParams, CreateWorkspaceRequest, ProvisionerLogLevel, UsageAppName, @@ -266,12 +267,12 @@ export const startWorkspace = ( export const cancelBuild = (workspace: Workspace, queryClient: QueryClient) => { return { mutationFn: () => { - if (workspace.latest_build.status === "pending") { - return API.cancelWorkspaceBuild(workspace.latest_build.id, { - expect_status: "pending", - }); - } - return API.cancelWorkspaceBuild(workspace.latest_build.id); + const { status } = workspace.latest_build; + const params: CancelWorkspaceBuildParams = { + expect_status: + status === "pending" || status === "running" ? status : undefined, + }; + return API.cancelWorkspaceBuild(workspace.latest_build.id, params); }, onSuccess: async () => { await queryClient.invalidateQueries({ From 634f556902fefb5b8ee068e3ab6b65914546a4a9 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Mon, 7 Jul 2025 11:04:56 +0000 Subject: [PATCH 16/19] Refactor cancelWorkspaceBuild parameter handling --- site/src/api/queries/workspaces.ts | 8 ++++---- site/src/pages/WorkspacePage/WorkspacePage.test.tsx | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/site/src/api/queries/workspaces.ts b/site/src/api/queries/workspaces.ts index a8f9b0e8f49ee..f5fabc3e0fbce 100644 --- a/site/src/api/queries/workspaces.ts +++ b/site/src/api/queries/workspaces.ts @@ -268,10 +268,10 @@ export const cancelBuild = (workspace: Workspace, queryClient: QueryClient) => { return { mutationFn: () => { const { status } = workspace.latest_build; - const params: CancelWorkspaceBuildParams = { - expect_status: - status === "pending" || status === "running" ? status : undefined, - }; + const params = + status === "pending" || status === "running" + ? { expect_status: status } + : undefined; return API.cancelWorkspaceBuild(workspace.latest_build.id, params); }, onSuccess: async () => { diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index 9581244e603fa..07ffb93722d73 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -244,6 +244,7 @@ describe("WorkspacePage", () => { expect(cancelWorkspaceMock).toHaveBeenCalledWith( MockStartingWorkspace.latest_build.id, + undefined, ); }); From 4deace0e15b57466bacdbdc86d5308fbc364f376 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Mon, 7 Jul 2025 11:14:04 +0000 Subject: [PATCH 17/19] Fix lint --- site/src/api/queries/workspaces.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/site/src/api/queries/workspaces.ts b/site/src/api/queries/workspaces.ts index f5fabc3e0fbce..05fb09314d741 100644 --- a/site/src/api/queries/workspaces.ts +++ b/site/src/api/queries/workspaces.ts @@ -1,7 +1,6 @@ import { API, type DeleteWorkspaceOptions } from "api/api"; import { DetailedError, isApiValidationError } from "api/errors"; import type { - CancelWorkspaceBuildParams, CreateWorkspaceRequest, ProvisionerLogLevel, UsageAppName, From 43430fa5644edffa93d358f3bfd416e3cab09074 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Mon, 7 Jul 2025 14:30:13 +0200 Subject: [PATCH 18/19] Update coderd/workspacebuilds.go Co-authored-by: Dean Sheather --- coderd/workspacebuilds.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index dc58c75033f2a..aa0f10abe1b43 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -694,7 +694,8 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques } func verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID, jobStatus database.ProvisionerJobStatus) (bool, error) { - // If the jobStatus is pending, we can cancel it. + // If the jobStatus is pending, we always allow cancellation regardless of + // the template setting as it's non-destructive to Terraform resources. if jobStatus == database.ProvisionerJobStatusPending { return true, nil } From 6272d9391b4d86f3e8060a7a540d5c981746f6f4 Mon Sep 17 00:00:00 2001 From: Kacper Sawicki Date: Mon, 7 Jul 2025 13:54:21 +0000 Subject: [PATCH 19/19] Extract cancel confirm dialog to a separate component --- coderd/workspacebuilds.go | 2 +- .../WorkspaceBuildCancelDialog.tsx | 32 +++++++++++++++++++ .../WorkspacePage/WorkspaceReadyPage.tsx | 9 ++---- .../pages/WorkspacesPage/WorkspacesTable.tsx | 10 ++---- 4 files changed, 39 insertions(+), 14 deletions(-) create mode 100644 site/src/modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog.tsx diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index aa0f10abe1b43..c8b1008280b09 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -695,7 +695,7 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques func verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID, jobStatus database.ProvisionerJobStatus) (bool, error) { // If the jobStatus is pending, we always allow cancellation regardless of - // the template setting as it's non-destructive to Terraform resources. + // the template setting as it's non-destructive to Terraform resources. if jobStatus == database.ProvisionerJobStatusPending { return true, nil } diff --git a/site/src/modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog.tsx b/site/src/modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog.tsx new file mode 100644 index 0000000000000..cbd7eb7d0c1ed --- /dev/null +++ b/site/src/modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog.tsx @@ -0,0 +1,32 @@ +import type { Workspace } from "api/typesGenerated"; +import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog"; +import type { FC } from "react"; + +interface WorkspaceBuildCancelDialogProps { + open: boolean; + onClose: () => void; + onConfirm: () => void; + workspace: Workspace; +} + +export const WorkspaceBuildCancelDialog: FC< + WorkspaceBuildCancelDialogProps +> = ({ open, onClose, onConfirm, workspace }) => { + const action = + workspace.latest_build.status === "pending" + ? "remove the current build from the build queue" + : "stop the current build process"; + + return ( + + ); +}; diff --git a/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx b/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx index c46d73376ca5b..4034cc144e127 100644 --- a/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx @@ -20,6 +20,7 @@ import { displayError } from "components/GlobalSnackbar/utils"; import { useWorkspaceBuildLogs } from "hooks/useWorkspaceBuildLogs"; import { EphemeralParametersDialog } from "modules/workspaces/EphemeralParametersDialog/EphemeralParametersDialog"; import { WorkspaceErrorDialog } from "modules/workspaces/ErrorDialog/WorkspaceErrorDialog"; +import { WorkspaceBuildCancelDialog } from "modules/workspaces/WorkspaceBuildCancelDialog/WorkspaceBuildCancelDialog"; import { WorkspaceUpdateDialogs, useWorkspaceUpdate, @@ -354,18 +355,14 @@ export const WorkspaceReadyPage: FC = ({ } /> - setIsCancelConfirmOpen(false)} onConfirm={() => { cancelBuildMutation.mutate(); setIsCancelConfirmOpen(false); }} - type="delete" + workspace={workspace} /> = ({ type="delete" /> - {/* Cancel workspace build confirmation dialog */} - setIsCancelConfirmOpen(false)} onConfirm={() => { cancelBuildMutation.mutate(); setIsCancelConfirmOpen(false); }} - type="delete" + workspace={workspace} /> );