Skip to content

Commit 776231d

Browse files
kacpersawkacpersaw
authored
fix(coderd): add blocking GetProvisionerJobByIDWithLock for workspace build cancellation (#19737)
Closes coder/internal#885 Adds a new database method GetProvisionerJobByIDWithLock that uses FOR UPDATE without SKIP LOCKED to fix workspace build cancellation returning 500 errors when jobs are locked.
1 parent 065c7c3 commit 776231d

File tree

9 files changed

+109
-2
lines changed

9 files changed

+109
-2
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2610,6 +2610,18 @@ func (q *querier) GetProvisionerJobByIDForUpdate(ctx context.Context, id uuid.UU
26102610
return job, nil
26112611
}
26122612

2613+
func (q *querier) GetProvisionerJobByIDWithLock(ctx context.Context, id uuid.UUID) (database.ProvisionerJob, error) {
2614+
job, err := q.db.GetProvisionerJobByIDWithLock(ctx, id)
2615+
if err != nil {
2616+
return database.ProvisionerJob{}, err
2617+
}
2618+
2619+
if err := q.authorizeProvisionerJob(ctx, job); err != nil {
2620+
return database.ProvisionerJob{}, err
2621+
}
2622+
return job, nil
2623+
}
2624+
26132625
func (q *querier) GetProvisionerJobTimingsByJobID(ctx context.Context, jobID uuid.UUID) ([]database.ProvisionerJobTiming, error) {
26142626
_, err := q.GetProvisionerJobByID(ctx, jobID)
26152627
if err != nil {

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -664,6 +664,24 @@ func (s *MethodTestSuite) TestProvisionerJob() {
664664
dbm.EXPECT().GetProvisionerLogsAfterID(gomock.Any(), arg).Return([]database.ProvisionerJobLog{}, nil).AnyTimes()
665665
check.Args(arg).Asserts(ws, policy.ActionRead).Returns([]database.ProvisionerJobLog{})
666666
}))
667+
s.Run("Build/GetProvisionerJobByIDWithLock", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
668+
ws := testutil.Fake(s.T(), faker, database.Workspace{})
669+
j := testutil.Fake(s.T(), faker, database.ProvisionerJob{Type: database.ProvisionerJobTypeWorkspaceBuild})
670+
build := testutil.Fake(s.T(), faker, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: j.ID})
671+
dbm.EXPECT().GetProvisionerJobByIDWithLock(gomock.Any(), j.ID).Return(j, nil).AnyTimes()
672+
dbm.EXPECT().GetWorkspaceBuildByJobID(gomock.Any(), j.ID).Return(build, nil).AnyTimes()
673+
dbm.EXPECT().GetWorkspaceByID(gomock.Any(), build.WorkspaceID).Return(ws, nil).AnyTimes()
674+
check.Args(j.ID).Asserts(ws, policy.ActionRead).Returns(j)
675+
}))
676+
s.Run("TemplateVersion/GetProvisionerJobByIDWithLock", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
677+
tpl := testutil.Fake(s.T(), faker, database.Template{})
678+
j := testutil.Fake(s.T(), faker, database.ProvisionerJob{Type: database.ProvisionerJobTypeTemplateVersionImport})
679+
v := testutil.Fake(s.T(), faker, database.TemplateVersion{JobID: j.ID, TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true}})
680+
dbm.EXPECT().GetProvisionerJobByIDWithLock(gomock.Any(), j.ID).Return(j, nil).AnyTimes()
681+
dbm.EXPECT().GetTemplateVersionByJobID(gomock.Any(), j.ID).Return(v, nil).AnyTimes()
682+
dbm.EXPECT().GetTemplateByID(gomock.Any(), tpl.ID).Return(tpl, nil).AnyTimes()
683+
check.Args(j.ID).Asserts(v.RBACObject(tpl), policy.ActionRead).Returns(j)
684+
}))
667685
}
668686

669687
func (s *MethodTestSuite) TestLicense() {

‎coderd/database/dbmetrics/querymetrics.go

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

‎coderd/database/dbmock/dbmock.go

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

‎coderd/database/querier.go

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

‎coderd/database/queries.sql.go

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

‎coderd/database/queries/provisionerjobs.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,17 @@ WHERE
5555
FOR UPDATE
5656
SKIP LOCKED;
5757

58+
-- name: GetProvisionerJobByIDWithLock :one
59+
-- Gets a provisioner job by ID with exclusive lock.
60+
-- Blocks until the row is available for update.
61+
SELECT
62+
*
63+
FROM
64+
provisioner_jobs
65+
WHERE
66+
id = $1
67+
FOR UPDATE;
68+
5869
-- name: GetProvisionerJobsByIDs :many
5970
SELECT
6071
*

‎coderd/workspacebuilds.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -663,7 +663,7 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques
663663
return xerrors.New("user is not allowed to cancel workspace builds")
664664
}
665665

666-
job, err := db.GetProvisionerJobByIDForUpdate(ctx, workspaceBuild.JobID)
666+
job, err := db.GetProvisionerJobByIDWithLock(ctx, workspaceBuild.JobID)
667667
if err != nil {
668668
code = http.StatusInternalServerError
669669
resp.Message = "Internal error fetching provisioner job."

‎coderd/workspacebuilds_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -580,7 +580,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) {
580580

581581
require.Eventually(t, func() bool {
582582
err := client.CancelWorkspaceBuild(ctx, build.ID, codersdk.CancelWorkspaceBuildParams{})
583-
return assert.NoError(t, err)
583+
return err == nil
584584
}, testutil.WaitShort, testutil.IntervalMedium)
585585

586586
require.Eventually(t, func() bool {

0 commit comments

Comments
 (0)