Skip to content

Commit c4ee5b6

Browse files
kacpersawkacpersaw
committed
Use single transaction for canceling workspace build
1 parent ba41ae8 commit c4ee5b6

File tree

6 files changed

+82
-83
lines changed

6 files changed

+82
-83
lines changed

‎coderd/apidoc/docs.go

Lines changed: 6 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 3 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/workspacebuilds.go

Lines changed: 49 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -581,12 +581,12 @@ func (api *API) notifyWorkspaceUpdated(
581581
// @Produce json
582582
// @Tags Builds
583583
// @Param workspacebuild path string true "Workspace build ID"
584-
// @Param expect_state query string false "Expected state of the job"
584+
// @Param expect_status query string false "Expected status of the job" Enums(running, pending)
585585
// @Success 200 {object} codersdk.Response
586586
// @Router /workspacebuilds/{workspacebuild}/cancel [patch]
587587
func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Request) {
588588
ctx := r.Context()
589-
expectState := r.URL.Query().Get("expect_state")
589+
expectStatus := r.URL.Query().Get("expect_status")
590590
workspaceBuild := httpmw.WorkspaceBuildParam(r)
591591
workspace, err := api.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID)
592592
if err != nil {
@@ -596,90 +596,60 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
596596
return
597597
}
598598

599-
valid, err := api.verifyUserCanCancelWorkspaceBuilds(ctx, httpmw.APIKey(r).UserID, workspace.TemplateID)
600-
if err != nil {
601-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
602-
Message: "Internal error verifying permission to cancel workspace build.",
603-
Detail: err.Error(),
604-
})
605-
return
606-
}
607-
if !valid {
608-
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
609-
Message: "User is not allowed to cancel workspace builds. Owner role is required.",
610-
})
611-
return
612-
}
613-
614-
if expectState != "" {
615-
jobStatus := database.ProvisionerJobStatus(expectState)
616-
if !jobStatus.Valid() {
617-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
618-
Message: "Invalid expect_state. Only 'pending', 'running', 'succeeded', 'canceling', 'canceled', 'failed', or 'unknown' are allowed.",
619-
})
620-
return
621-
}
622-
623-
// use local error type to detect if the error is due to job state mismatch
624-
var errJobStateMismatch = xerrors.New("job is not in the expected state")
625-
626-
err := api.Database.InTx(func(store database.Store) error {
627-
job, err := store.GetProvisionerJobByIDForUpdate(ctx, workspaceBuild.JobID)
628-
if err != nil {
629-
return err
630-
}
631-
632-
if job.JobStatus != jobStatus {
633-
return errJobStateMismatch
634-
}
635-
636-
return store.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{
637-
ID: job.ID,
638-
CanceledAt: sql.NullTime{
639-
Time: dbtime.Now(),
640-
Valid: true,
641-
},
642-
CompletedAt: sql.NullTime{
643-
Time: dbtime.Now(),
644-
Valid: !job.WorkerID.Valid,
645-
},
646-
})
647-
}, nil)
599+
err = api.Database.InTx(func(store database.Store) error {
600+
valid, err := api.verifyUserCanCancelWorkspaceBuilds(ctx, store, httpmw.APIKey(r).UserID, workspace.TemplateID)
648601
if err != nil {
649-
if errors.Is(err, errJobStateMismatch) {
650-
httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{
651-
Message: "Job is not in the expected state.",
652-
})
653-
return
654-
}
655602
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
656-
Message: "Internal error fetching provisioner job.",
603+
Message: "Internal error verifying permission to cancel workspace build.",
657604
Detail: err.Error(),
658605
})
659-
return
606+
return nil
607+
}
608+
if !valid {
609+
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
610+
Message: "User is not allowed to cancel workspace builds. Owner role is required.",
611+
})
612+
return nil
660613
}
661-
} else {
662-
job, err := api.Database.GetProvisionerJobByID(ctx, workspaceBuild.JobID)
614+
615+
job, err := store.GetProvisionerJobByID(ctx, workspaceBuild.JobID)
663616
if err != nil {
664617
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
665618
Message: "Internal error fetching provisioner job.",
666619
Detail: err.Error(),
667620
})
668-
return
621+
return nil
669622
}
670623
if job.CompletedAt.Valid {
671624
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
672625
Message: "Job has already completed!",
673626
})
674-
return
627+
return nil
675628
}
676629
if job.CanceledAt.Valid {
677630
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
678631
Message: "Job has already been marked as canceled!",
679632
})
680-
return
633+
return nil
634+
}
635+
636+
if expectStatus != "" {
637+
if expectStatus != "running" && expectStatus != "pending" {
638+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
639+
Message: "Invalid expect_status. Only 'running' or 'pending' are allowed.",
640+
})
641+
return nil
642+
}
643+
644+
if job.JobStatus != database.ProvisionerJobStatus(expectStatus) {
645+
httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{
646+
Message: "Job is not in the expected state.",
647+
})
648+
return nil
649+
}
681650
}
682-
err = api.Database.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{
651+
652+
err = store.UpdateProvisionerJobWithCancelByID(ctx, database.UpdateProvisionerJobWithCancelByIDParams{
683653
ID: job.ID,
684654
CanceledAt: sql.NullTime{
685655
Time: dbtime.Now(),
@@ -696,9 +666,19 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
696666
Message: "Internal error updating provisioner job.",
697667
Detail: err.Error(),
698668
})
699-
return
669+
return nil
700670
}
671+
672+
return nil
673+
}, nil)
674+
if err != nil {
675+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
676+
Message: "Internal error updating provisioner job.",
677+
Detail: err.Error(),
678+
})
679+
return
701680
}
681+
702682
api.publishWorkspaceUpdate(ctx, workspace.OwnerID, wspubsub.WorkspaceEvent{
703683
Kind: wspubsub.WorkspaceEventKindStateChange,
704684
WorkspaceID: workspace.ID,
@@ -709,8 +689,8 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
709689
})
710690
}
711691

712-
func (api *API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, userID uuid.UUID, templateID uuid.UUID) (bool, error) {
713-
template, err := api.Database.GetTemplateByID(ctx, templateID)
692+
func (*API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, store database.Store, userID uuid.UUID, templateID uuid.UUID) (bool, error) {
693+
template, err := store.GetTemplateByID(ctx, templateID)
714694
if err != nil {
715695
return false, xerrors.New("no template exists for this workspace")
716696
}
@@ -719,7 +699,7 @@ func (api *API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, userID u
719699
return true, nil // all users can cancel workspace builds
720700
}
721701

722-
user, err := api.Database.GetUserByID(ctx, userID)
702+
user, err := store.GetUserByID(ctx, userID)
723703
if err != nil {
724704
return false, xerrors.New("user does not exist")
725705
}

‎coderd/workspacebuilds_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -672,7 +672,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
672672

673673
// When: the workspace build is canceled
674674
err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{
675-
ExpectState: codersdk.ProvisionerJobPending,
675+
ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending,
676676
})
677677
require.NoError(t, err)
678678

@@ -734,7 +734,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
734734

735735
// When: a cancel request is made with expect_state=pending
736736
err = client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildRequest{
737-
ExpectState: codersdk.ProvisionerJobPending,
737+
ExpectStatus: codersdk.CancelWorkspaceBuildStatusPending,
738738
})
739739
// Then: the request should fail with 412.
740740
require.Error(t, err)
@@ -765,13 +765,13 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
765765

766766
// When: a cancel request is made with invalid expect_state
767767
err := client.CancelWorkspaceBuild(ctx, workspace.LatestBuild.ID, codersdk.CancelWorkspaceBuildRequest{
768-
ExpectState: "invalid_status",
768+
ExpectStatus: "invalid_status",
769769
})
770770
// Then: the request should fail with 400.
771771
var apiErr *codersdk.Error
772772
require.ErrorAs(t, err, &apiErr)
773773
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
774-
require.Contains(t, apiErr.Message, "Invalid expect_state")
774+
require.Contains(t, apiErr.Message, "Invalid expect_status")
775775
})
776776
}
777777

‎codersdk/workspacebuilds.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,14 +123,21 @@ func (c *Client) WorkspaceBuild(ctx context.Context, id uuid.UUID) (WorkspaceBui
123123
return workspaceBuild, json.NewDecoder(res.Body).Decode(&workspaceBuild)
124124
}
125125

126+
type CancelWorkspaceBuildStatus string
127+
128+
const (
129+
CancelWorkspaceBuildStatusRunning CancelWorkspaceBuildStatus = "running"
130+
CancelWorkspaceBuildStatusPending CancelWorkspaceBuildStatus = "pending"
131+
)
132+
126133
type CancelWorkspaceBuildRequest struct {
127-
ExpectState ProvisionerJobStatus `json:"expect_state,omitempty"`
134+
ExpectStatus CancelWorkspaceBuildStatus `json:"expect_status,omitempty"`
128135
}
129136

130137
func (c *CancelWorkspaceBuildRequest) asRequestOption() RequestOption {
131138
return func(r *http.Request) {
132139
q := r.URL.Query()
133-
q.Set("expect_state", string(c.ExpectState))
140+
q.Set("expect_status", string(c.ExpectStatus))
134141
r.URL.RawQuery = q.Encode()
135142
}
136143
}

‎docs/reference/api/builds.md

Lines changed: 11 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)