coder · Emyrk · Oct 5, 2023 · Oct 3, 2023 · Oct 3, 2023 · Oct 3, 2023
diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go
@@ -13,7 +13,6 @@ import (
 
 	"cdr.dev/slog"
 	"github.com/coder/coder/v2/coderd/database"
-	"github.com/coder/coder/v2/coderd/database/db2sdk"
 	"github.com/coder/coder/v2/coderd/database/dbauthz"
 	"github.com/coder/coder/v2/coderd/database/dbtime"
 	"github.com/coder/coder/v2/coderd/database/provisionerjobs"
@@ -303,7 +302,7 @@ func getNextTransition(
 // isEligibleForAutostart returns true if the workspace should be autostarted.
 func isEligibleForAutostart(ws database.Workspace, build database.WorkspaceBuild, job database.ProvisionerJob, templateSchedule schedule.TemplateScheduleOptions, currentTick time.Time) bool {
 	// Don't attempt to autostart failed workspaces.
-	if db2sdk.ProvisionerJobStatus(job) == codersdk.ProvisionerJobFailed {
+	if codersdk.ProvisionerJobStatus(job.JobStatus) == codersdk.ProvisionerJobFailed {
 		return false
 	}
 
@@ -337,7 +336,7 @@ func isEligibleForAutostart(ws database.Workspace, build database.WorkspaceBuild
 
 // isEligibleForAutostart returns true if the workspace should be autostopped.
 func isEligibleForAutostop(ws database.Workspace, build database.WorkspaceBuild, job database.ProvisionerJob, currentTick time.Time) bool {
-	if db2sdk.ProvisionerJobStatus(job) == codersdk.ProvisionerJobFailed {
+	if codersdk.ProvisionerJobStatus(job.JobStatus) == codersdk.ProvisionerJobFailed {
 		return false
 	}
 
@@ -379,7 +378,7 @@ func isEligibleForFailedStop(build database.WorkspaceBuild, job database.Provisi
 	// If the template has specified a failure TLL.
 	return templateSchedule.FailureTTL > 0 &&
 		// And the job resulted in failure.
-		db2sdk.ProvisionerJobStatus(job) == codersdk.ProvisionerJobFailed &&
+		codersdk.ProvisionerJobStatus(job.JobStatus) == codersdk.ProvisionerJobFailed &&
 		build.Transition == database.WorkspaceTransitionStart &&
 		// And sufficient time has elapsed since the job has completed.
 		job.CompletedAt.Valid &&

diff --git a/coderd/database/db2sdk/db2sdk.go b/coderd/database/db2sdk/db2sdk.go
@@ -71,31 +71,6 @@ func TemplateVersionParameter(param database.TemplateVersionParameter) (codersdk
 	}, nil
 }
 
-func ProvisionerJobStatus(provisionerJob database.ProvisionerJob) codersdk.ProvisionerJobStatus {
-	// The case where jobs are hung is handled by the unhang package. We can't
-	// just return Failed here when it's hung because that doesn't reflect in
-	// the database.
-	switch {
-	case provisionerJob.CanceledAt.Valid:
-		if !provisionerJob.CompletedAt.Valid {
-			return codersdk.ProvisionerJobCanceling
-		}
-		if provisionerJob.Error.String == "" {
-			return codersdk.ProvisionerJobCanceled
-		}
-		return codersdk.ProvisionerJobFailed
-	case !provisionerJob.StartedAt.Valid:
-		return codersdk.ProvisionerJobPending
-	case provisionerJob.CompletedAt.Valid:
-		if provisionerJob.Error.String == "" {
-			return codersdk.ProvisionerJobSucceeded
-		}
-		return codersdk.ProvisionerJobFailed
-	default:
-		return codersdk.ProvisionerJobRunning
-	}
-}
-
 func User(user database.User, organizationIDs []uuid.UUID) codersdk.User {
 	convertedUser := codersdk.User{
 		ID:              user.ID,

diff --git a/coderd/database/db2sdk/db2sdk_test.go b/coderd/database/db2sdk/db2sdk_test.go
@@ -4,13 +4,17 @@ import (
 	"crypto/rand"
 	"database/sql"
 	"encoding/json"
+	"fmt"
 	"testing"
 	"time"
 
+	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/db2sdk"
+	"github.com/coder/coder/v2/coderd/database/dbgen"
+	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/database/dbtime"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/provisionersdk/proto"
@@ -110,11 +114,36 @@ func TestProvisionerJobStatus(t *testing.T) {
 		},
 	}
 
-	for _, tc := range cases {
+	// Share db for all job inserts.
+	db, _ := dbtestutil.NewDB(t)
+	org := dbgen.Organization(t, db, database.Organization{})
+
+	for i, tc := range cases {
 		tc := tc
+		i := i
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			actual := db2sdk.ProvisionerJobStatus(tc.job)
+			// Populate standard fields
+			now := dbtime.Now().Round(time.Minute)
+			tc.job.ID = uuid.New()
+			tc.job.CreatedAt = now
+			tc.job.UpdatedAt = now
+			tc.job.InitiatorID = org.ID
+			tc.job.OrganizationID = org.ID
+			tc.job.Input = []byte("{}")
+			tc.job.Provisioner = database.ProvisionerTypeEcho
+			// Unique tags for each job.
+			tc.job.Tags = map[string]string{fmt.Sprintf("%d", i): "true"}
+
+			inserted := dbgen.ProvisionerJob(t, db, nil, tc.job)
+			// Make sure the inserted job has the right values.
+			require.Equal(t, tc.job.StartedAt.Time.UTC(), inserted.StartedAt.Time.UTC(), "started at")
+			require.Equal(t, tc.job.CompletedAt.Time.UTC(), inserted.CompletedAt.Time.UTC(), "completed at")
+			require.Equal(t, tc.job.CanceledAt.Time.UTC(), inserted.CanceledAt.Time.UTC(), "cancelled at")
+			require.Equal(t, tc.job.Error, inserted.Error, "error")
+			require.Equal(t, tc.job.ErrorCode, inserted.ErrorCode, "error code")
+
+			actual := codersdk.ProvisionerJobStatus(inserted.JobStatus)
 			require.Equal(t, tc.status, actual)
 		})
 	}

diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go
@@ -20,7 +20,6 @@ import (
 	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/v2/coderd/database"
-	"github.com/coder/coder/v2/coderd/database/db2sdk"
 	"github.com/coder/coder/v2/coderd/database/dbtime"
 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/coderd/rbac"
@@ -604,16 +603,6 @@ func (q *FakeQuerier) getGroupByIDNoLock(_ context.Context, id uuid.UUID) (datab
 	return database.Group{}, sql.ErrNoRows
 }
 
-// isNull is only used in dbfake, so reflect is ok. Use this to make the logic
-// look more similar to the postgres.
-func isNull(v interface{}) bool {
-	return !isNotNull(v)
-}
-
-func isNotNull(v interface{}) bool {
-	return reflect.ValueOf(v).FieldByName("Valid").Bool()
-}
-
 // ErrUnimplemented is returned by methods only used by the enterprise/tailnet.pgCoord.  This coordinator explicitly
 // depends on  postgres triggers that announce changes on the pubsub.  Implementing support for this in the fake
 // database would  strongly couple the FakeQuerier to the pubsub, which is undesirable.  Furthermore, it makes little
@@ -695,6 +684,36 @@ func minTime(t, u time.Time) time.Time {
 	return u
 }
 
+func provisonerJobStatus(j database.ProvisionerJob) database.ProvisionerJobStatus {
+	if isNotNull(j.CompletedAt) {
+		if j.Error.String != "" {
+			return database.ProvisionerJobStatusFailed
+		}
+		if isNotNull(j.CanceledAt) {
+			return database.ProvisionerJobStatusCanceled
+		}
+		return database.ProvisionerJobStatusSucceeded
+	}
+
+	if isNotNull(j.CanceledAt) {
+		return database.ProvisionerJobStatusCanceling
+	}
+	if isNull(j.StartedAt) {
+		return database.ProvisionerJobStatusPending
+	}
+	return database.ProvisionerJobStatusRunning
+}
+
+// isNull is only used in dbfake, so reflect is ok. Use this to make the logic
+// look more similar to the postgres.
+func isNull(v interface{}) bool {
+	return !isNotNull(v)
+}
+
+func isNotNull(v interface{}) bool {
+	return reflect.ValueOf(v).FieldByName("Valid").Bool()
+}
+
 func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error {
 	return xerrors.New("AcquireLock must only be called within a transaction")
 }
@@ -748,6 +767,7 @@ func (q *FakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.Acqu
 		provisionerJob.StartedAt = arg.StartedAt
 		provisionerJob.UpdatedAt = arg.StartedAt.Time
 		provisionerJob.WorkerID = arg.WorkerID
+		provisionerJob.JobStatus = provisonerJobStatus(provisionerJob)
 		q.provisionerJobs[index] = provisionerJob
 		return provisionerJob, nil
 	}
@@ -4077,7 +4097,7 @@ func (q *FakeQuerier) GetWorkspacesEligibleForTransition(ctx context.Context, no
 		if err != nil {
 			return nil, xerrors.Errorf("get provisioner job by ID: %w", err)
 		}
-		if db2sdk.ProvisionerJobStatus(job) == codersdk.ProvisionerJobFailed {
+		if codersdk.ProvisionerJobStatus(job.JobStatus) == codersdk.ProvisionerJobFailed {
 			workspaces = append(workspaces, workspace)
 			continue
 		}
@@ -4464,6 +4484,7 @@ func (q *FakeQuerier) InsertProvisionerJob(_ context.Context, arg database.Inser
 		Input:          arg.Input,
 		Tags:           arg.Tags,
 	}
+	job.JobStatus = provisonerJobStatus(job)
 	q.provisionerJobs = append(q.provisionerJobs, job)
 	return job, nil
 }
@@ -5393,6 +5414,7 @@ func (q *FakeQuerier) UpdateProvisionerJobByID(_ context.Context, arg database.U
 			continue
 		}
 		job.UpdatedAt = arg.UpdatedAt
+		job.JobStatus = provisonerJobStatus(job)
 		q.provisionerJobs[index] = job
 		return nil
 	}
@@ -5413,6 +5435,7 @@ func (q *FakeQuerier) UpdateProvisionerJobWithCancelByID(_ context.Context, arg
 		}
 		job.CanceledAt = arg.CanceledAt
 		job.CompletedAt = arg.CompletedAt
+		job.JobStatus = provisonerJobStatus(job)
 		q.provisionerJobs[index] = job
 		return nil
 	}
@@ -5435,6 +5458,7 @@ func (q *FakeQuerier) UpdateProvisionerJobWithCompleteByID(_ context.Context, ar
 		job.CompletedAt = arg.CompletedAt
 		job.Error = arg.Error
 		job.ErrorCode = arg.ErrorCode
+		job.JobStatus = provisonerJobStatus(job)
 		q.provisionerJobs[index] = job
 		return nil
 	}
@@ -6604,61 +6628,30 @@ func (q *FakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
 			// This logic should match the logic in the workspace.sql file.
 			var statusMatch bool
 			switch database.WorkspaceStatus(arg.Status) {
-			case database.WorkspaceStatusPending:
-				statusMatch = isNull(job.StartedAt)
 			case database.WorkspaceStatusStarting:
-				statusMatch = isNotNull(job.StartedAt) &&
-					isNull(job.CanceledAt) &&
-					isNull(job.CompletedAt) &&
-					time.Since(job.UpdatedAt) < 30*time.Second &&
-					build.Transition == database.WorkspaceTransitionStart
-
-			case database.WorkspaceStatusRunning:
-				statusMatch = isNotNull(job.CompletedAt) &&
-					isNull(job.CanceledAt) &&
-					isNull(job.Error) &&
+				statusMatch = job.JobStatus == database.ProvisionerJobStatusRunning &&
 					build.Transition == database.WorkspaceTransitionStart
-
 			case database.WorkspaceStatusStopping:
-				statusMatch = isNotNull(job.StartedAt) &&
-					isNull(job.CanceledAt) &&
-					isNull(job.CompletedAt) &&
-					time.Since(job.UpdatedAt) < 30*time.Second &&
-					build.Transition == database.WorkspaceTransitionStop
-
-			case database.WorkspaceStatusStopped:
-				statusMatch = isNotNull(job.CompletedAt) &&
-					isNull(job.CanceledAt) &&
-					isNull(job.Error) &&
+				statusMatch = job.JobStatus == database.ProvisionerJobStatusRunning &&
 					build.Transition == database.WorkspaceTransitionStop
-			case database.WorkspaceStatusFailed:
-				statusMatch = (isNotNull(job.CanceledAt) && isNotNull(job.Error)) ||
-					(isNotNull(job.CompletedAt) && isNotNull(job.Error))
-
-			case database.WorkspaceStatusCanceling:
-				statusMatch = isNotNull(job.CanceledAt) &&
-					isNull(job.CompletedAt)
-
-			case database.WorkspaceStatusCanceled:
-				statusMatch = isNotNull(job.CanceledAt) &&
-					isNotNull(job.CompletedAt)
-
-			case database.WorkspaceStatusDeleted:
-				statusMatch = isNotNull(job.StartedAt) &&
-					isNull(job.CanceledAt) &&
-					isNotNull(job.CompletedAt) &&
-					time.Since(job.UpdatedAt) < 30*time.Second &&
-					build.Transition == database.WorkspaceTransitionDelete &&
-					isNull(job.Error)
-
 			case database.WorkspaceStatusDeleting:
-				statusMatch = isNull(job.CompletedAt) &&
-					isNull(job.CanceledAt) &&
-					isNull(job.Error) &&
+				statusMatch = job.JobStatus == database.ProvisionerJobStatusRunning &&
 					build.Transition == database.WorkspaceTransitionDelete
 
+			case "started":
+				statusMatch = job.JobStatus == database.ProvisionerJobStatusSucceeded &&
+					build.Transition == database.WorkspaceTransitionStart
+			case database.WorkspaceStatusDeleted:
+				statusMatch = job.JobStatus == database.ProvisionerJobStatusSucceeded &&
+					build.Transition == database.WorkspaceTransitionDelete
+			case database.WorkspaceStatusStopped:
+				statusMatch = job.JobStatus == database.ProvisionerJobStatusSucceeded &&
+					build.Transition == database.WorkspaceTransitionStop
+			case database.WorkspaceStatusRunning:
+				statusMatch = job.JobStatus == database.ProvisionerJobStatusSucceeded &&
+					build.Transition == database.WorkspaceTransitionStart
 			default:
-				return nil, xerrors.Errorf("unknown workspace status in filter: %q", arg.Status)
+				statusMatch = job.JobStatus == database.ProvisionerJobStatus(arg.Status)
 			}
 			if !statusMatch {
 				continue

diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go
@@ -328,16 +328,16 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) dat
 // ProvisionerJob is a bit more involved to get the values such as "completedAt", "startedAt", "cancelledAt" set.  ps
 // can be set to nil if you are SURE that you don't require a provisionerdaemon to acquire the job in your test.
 func ProvisionerJob(t testing.TB, db database.Store, ps pubsub.Pubsub, orig database.ProvisionerJob) database.ProvisionerJob {
-	id := takeFirst(orig.ID, uuid.New())
+	jobID := takeFirst(orig.ID, uuid.New())
 	// Always set some tags to prevent Acquire from grabbing jobs it should not.
 	if !orig.StartedAt.Time.IsZero() {
 		if orig.Tags == nil {
 			orig.Tags = make(database.StringMap)
 		}
 		// Make sure when we acquire the job, we only get this one.
-		orig.Tags[id.String()] = "true"
+		orig.Tags[jobID.String()] = "true"
 	}
-	jobID := takeFirst(orig.ID, uuid.New())
+
 	job, err := db.InsertProvisionerJob(genCtx, database.InsertProvisionerJobParams{
 		ID:             jobID,
 		CreatedAt:      takeFirst(orig.CreatedAt, dbtime.Now()),
@@ -365,6 +365,8 @@ func ProvisionerJob(t testing.TB, db database.Store, ps pubsub.Pubsub, orig data
 			WorkerID:  uuid.NullUUID{},
 		})
 		require.NoError(t, err)
+		// There is no easy way to make sure we acquire the correct job.
+		require.Equal(t, jobID, job.ID, "acquired incorrect job")
 	}
 
 	if !orig.CompletedAt.Time.IsZero() || orig.Error.String != "" {