coder · spikecurtis · May 18, 2022 · May 16, 2022 · May 17, 2022 · May 17, 2022
diff --git a/cli/resetpassword_test.go b/cli/resetpassword_test.go
@@ -15,8 +15,10 @@ import (
 	"github.com/coder/coder/pty/ptytest"
 )
 
+// nolint:paralleltest
 func TestResetPassword(t *testing.T) {
-	t.Parallel()
+	// postgres.Open() seems to be creating race conditions when run in parallel.
+	// t.Parallel()
 
 	if runtime.GOOS != "linux" || testing.Short() {
 		// Skip on non-Linux because it spawns a PostgreSQL instance.

diff --git a/cli/server_test.go b/cli/server_test.go
@@ -32,10 +32,11 @@ import (
 )
 
 // This cannot be ran in parallel because it uses a signal.
-// nolint:tparallel
+// nolint:paralleltest
 func TestServer(t *testing.T) {
 	t.Run("Production", func(t *testing.T) {
-		t.Parallel()
+		// postgres.Open() seems to be creating race conditions when run in parallel.
+		// t.Parallel()
 		if runtime.GOOS != "linux" || testing.Short() {
 			// Skip on non-Linux because it spawns a PostgreSQL instance.
 			t.SkipNow()

diff --git a/coderd/autobuild/executor/lifecycle_executor.go b/coderd/autobuild/executor/lifecycle_executor.go
@@ -57,7 +57,7 @@ func (e *Executor) runOnce(t time.Time) error {
 
 		for _, ws := range eligibleWorkspaces {
 			// Determine the workspace state based on its latest build.
-			priorHistory, err := db.GetWorkspaceBuildByWorkspaceIDWithoutAfter(e.ctx, ws.ID)
+			priorHistory, err := db.GetLatestWorkspaceBuildByWorkspaceID(e.ctx, ws.ID)
 			if err != nil {
 				e.log.Warn(e.ctx, "get latest workspace build",
 					slog.F("workspace_id", ws.ID),
@@ -152,12 +152,8 @@ func build(ctx context.Context, store database.Store, workspace database.Workspa
 		return xerrors.Errorf("get workspace template: %w", err)
 	}
 
-	priorHistoryID := uuid.NullUUID{
-		UUID:  priorHistory.ID,
-		Valid: true,
-	}
+	priorBuildNumber := priorHistory.BuildNumber
 
-	var newWorkspaceBuild database.WorkspaceBuild
 	// This must happen in a transaction to ensure history can be inserted, and
 	// the prior history can update it's "after" column to point at the new.
 	workspaceBuildID := uuid.New()
@@ -186,13 +182,13 @@ func build(ctx context.Context, store database.Store, workspace database.Workspa
 	if err != nil {
 		return xerrors.Errorf("insert provisioner job: %w", err)
 	}
-	newWorkspaceBuild, err = store.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{
+	_, err = store.InsertWorkspaceBuild(ctx, database.InsertWorkspaceBuildParams{
 		ID:                workspaceBuildID,
 		CreatedAt:         now,
 		UpdatedAt:         now,
 		WorkspaceID:       workspace.ID,
 		TemplateVersionID: priorHistory.TemplateVersionID,
-		BeforeID:          priorHistoryID,
+		BuildNumber:       priorBuildNumber + 1,
 		Name:              namesgenerator.GetRandomName(1),
 		ProvisionerState:  priorHistory.ProvisionerState,
 		InitiatorID:       workspace.OwnerID,
@@ -202,21 +198,5 @@ func build(ctx context.Context, store database.Store, workspace database.Workspa
 	if err != nil {
 		return xerrors.Errorf("insert workspace build: %w", err)
 	}
-
-	if priorHistoryID.Valid {
-		// Update the prior history entries "after" column.
-		err = store.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{
-			ID:               priorHistory.ID,
-			ProvisionerState: priorHistory.ProvisionerState,
-			UpdatedAt:        now,
-			AfterID: uuid.NullUUID{
-				UUID:  newWorkspaceBuild.ID,
-				Valid: true,
-			},
-		})
-		if err != nil {
-			return xerrors.Errorf("update prior workspace build: %w", err)
-		}
-	}
 	return nil
 }
diff --git a/coderd/autobuild/executor/lifecycle_executor_test.go b/coderd/autobuild/executor/lifecycle_executor_test.go
@@ -419,10 +419,17 @@ func TestExecutorAutostartMultipleOK(t *testing.T) {
 	require.NotEqual(t, workspace.LatestBuild.ID, ws.LatestBuild.ID, "expected a workspace build to occur")
 	require.Equal(t, codersdk.ProvisionerJobSucceeded, ws.LatestBuild.Job.Status, "expected provisioner job to have succeeded")
 	require.Equal(t, database.WorkspaceTransitionStart, ws.LatestBuild.Transition, "expected latest transition to be start")
-	builds, err := client.WorkspaceBuilds(ctx, ws.ID)
+	builds, err := client.WorkspaceBuilds(ctx, codersdk.WorkspaceBuildsRequest{WorkspaceID: ws.ID})
 	require.NoError(t, err, "fetch list of workspace builds from primary")
 	// One build to start, one stop transition, and one autostart. No more.
+	require.Equal(t, database.WorkspaceTransitionStart, builds[0].Transition)
+	require.Equal(t, database.WorkspaceTransitionStop, builds[1].Transition)
+	require.Equal(t, database.WorkspaceTransitionStart, builds[2].Transition)
 	require.Len(t, builds, 3, "unexpected number of builds for workspace from primary")
+
+	// Builds are returned most recent first.
+	require.True(t, builds[0].CreatedAt.After(builds[1].CreatedAt))
+	require.True(t, builds[1].CreatedAt.After(builds[2].CreatedAt))
 }
 
 func mustProvisionWorkspace(t *testing.T, client *codersdk.Client) codersdk.Workspace {

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -295,6 +295,20 @@ func CreateTemplateVersion(t *testing.T, client *codersdk.Client, organizationID
 	return templateVersion
 }
 
+// CreateWorkspaceBuild creates a workspace build for the given workspace and transition.
+func CreateWorkspaceBuild(
+	t *testing.T,
+	client *codersdk.Client,
+	workspace codersdk.Workspace,
+	transition database.WorkspaceTransition) codersdk.WorkspaceBuild {
+	req := codersdk.CreateWorkspaceBuildRequest{
+		Transition: transition,
+	}
+	build, err := client.CreateWorkspaceBuild(context.Background(), workspace.ID, req)
+	require.NoError(t, err)
+	return build
+}
+
 // CreateTemplate creates a template with the "echo" provisioner for
 // compatibility with testing. The name assigned is randomly generated.
 func CreateTemplate(t *testing.T, client *codersdk.Client, organization uuid.UUID, version uuid.UUID) codersdk.Template {

diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go
@@ -441,50 +441,100 @@ func (q *fakeQuerier) GetWorkspaceBuildByJobID(_ context.Context, jobID uuid.UUI
 	return database.WorkspaceBuild{}, sql.ErrNoRows
 }
 
-func (q *fakeQuerier) GetWorkspaceBuildByWorkspaceIDWithoutAfter(_ context.Context, workspaceID uuid.UUID) (database.WorkspaceBuild, error) {
+func (q *fakeQuerier) GetLatestWorkspaceBuildByWorkspaceID(_ context.Context, workspaceID uuid.UUID) (database.WorkspaceBuild, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
+	var row database.WorkspaceBuild
+	var buildNum int32
 	for _, workspaceBuild := range q.workspaceBuilds {
-		if workspaceBuild.WorkspaceID.String() != workspaceID.String() {
-			continue
-		}
-		if !workspaceBuild.AfterID.Valid {
-			return workspaceBuild, nil
+		if workspaceBuild.WorkspaceID.String() == workspaceID.String() && workspaceBuild.BuildNumber > buildNum {
+			row = workspaceBuild
+			buildNum = workspaceBuild.BuildNumber
 		}
 	}
-	return database.WorkspaceBuild{}, sql.ErrNoRows
+	if buildNum == 0 {
+		return database.WorkspaceBuild{}, sql.ErrNoRows
+	}
+	return row, nil
 }
 
-func (q *fakeQuerier) GetWorkspaceBuildsByWorkspaceIDsWithoutAfter(_ context.Context, ids []uuid.UUID) ([]database.WorkspaceBuild, error) {
+func (q *fakeQuerier) GetLatestWorkspaceBuildsByWorkspaceIDs(_ context.Context, ids []uuid.UUID) ([]database.WorkspaceBuild, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
-	builds := make([]database.WorkspaceBuild, 0)
+	builds := make(map[uuid.UUID]database.WorkspaceBuild)
+	buildNumbers := make(map[uuid.UUID]int32)
 	for _, workspaceBuild := range q.workspaceBuilds {
 		for _, id := range ids {
-			if id.String() != workspaceBuild.WorkspaceID.String() {
-				continue
+			if id.String() == workspaceBuild.WorkspaceID.String() && workspaceBuild.BuildNumber > buildNumbers[id] {
+				builds[id] = workspaceBuild
+				buildNumbers[id] = workspaceBuild.BuildNumber
 			}
-			builds = append(builds, workspaceBuild)
 		}
 	}
-	if len(builds) == 0 {
+	var returnBuilds []database.WorkspaceBuild
+	for i, n := range buildNumbers {
+		if n > 0 {
+			b := builds[i]
+			returnBuilds = append(returnBuilds, b)
+		}
+	}
+	if len(returnBuilds) == 0 {
 		return nil, sql.ErrNoRows
 	}
-	return builds, nil
+	return returnBuilds, nil
 }
 
-func (q *fakeQuerier) GetWorkspaceBuildByWorkspaceID(_ context.Context, workspaceID uuid.UUID) ([]database.WorkspaceBuild, error) {
+func (q *fakeQuerier) GetWorkspaceBuildByWorkspaceID(_ context.Context,
+	params database.GetWorkspaceBuildByWorkspaceIDParams) ([]database.WorkspaceBuild, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
 	history := make([]database.WorkspaceBuild, 0)
 	for _, workspaceBuild := range q.workspaceBuilds {
-		if workspaceBuild.WorkspaceID.String() == workspaceID.String() {
+		if workspaceBuild.WorkspaceID.String() == params.WorkspaceID.String() {
 			history = append(history, workspaceBuild)
 		}
 	}
+
+	// Order by build_number
+	slices.SortFunc(history, func(a, b database.WorkspaceBuild) bool {
+		// use greater than since we want descending order
+		return a.BuildNumber > b.BuildNumber
+	})
+
+	if params.AfterID != uuid.Nil {
+		found := false
+		for i, v := range history {
+			if v.ID == params.AfterID {
+				// We want to return all builds after index i.
+				history = history[i+1:]
+				found = true
+				break
+			}
+		}
+
+		// If no builds after the time, then we return an empty list.
+		if !found {
+			return nil, sql.ErrNoRows
+		}
+	}
+
+	if params.OffsetOpt > 0 {
+		if int(params.OffsetOpt) > len(history)-1 {
+			return nil, sql.ErrNoRows
+		}
+		history = history[params.OffsetOpt:]
+	}
+
+	if params.LimitOpt > 0 {
+		if int(params.LimitOpt) > len(history) {
+			params.LimitOpt = int32(len(history))
+		}
+		history = history[:params.LimitOpt]
+	}
+
 	if len(history) == 0 {
 		return nil, sql.ErrNoRows
 	}
@@ -1429,7 +1479,7 @@ func (q *fakeQuerier) InsertWorkspaceBuild(_ context.Context, arg database.Inser
 		WorkspaceID:       arg.WorkspaceID,
 		Name:              arg.Name,
 		TemplateVersionID: arg.TemplateVersionID,
-		BeforeID:          arg.BeforeID,
+		BuildNumber:       arg.BuildNumber,
 		Transition:        arg.Transition,
 		InitiatorID:       arg.InitiatorID,
 		JobID:             arg.JobID,
@@ -1641,7 +1691,6 @@ func (q *fakeQuerier) UpdateWorkspaceBuildByID(_ context.Context, arg database.U
 			continue
 		}
 		workspaceBuild.UpdatedAt = arg.UpdatedAt
-		workspaceBuild.AfterID = arg.AfterID
 		workspaceBuild.ProvisionerState = arg.ProvisionerState
 		q.workspaceBuilds[index] = workspaceBuild
 		return nil

diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000004_jobs.up.sql b/coderd/database/migrations/000004_jobs.up.sql
@@ -165,14 +165,14 @@ CREATE TABLE workspace_builds (
     workspace_id uuid NOT NULL REFERENCES workspaces (id) ON DELETE CASCADE,
     template_version_id uuid NOT NULL REFERENCES template_versions (id) ON DELETE CASCADE,
     name varchar(64) NOT NULL,
-    before_id uuid,
-    after_id uuid,
+    build_number integer NOT NULL,
     transition workspace_transition NOT NULL,
     initiator_id uuid NOT NULL,
     -- State stored by the provisioner
     provisioner_state bytea,
     -- Job ID of the action
     job_id uuid NOT NULL UNIQUE REFERENCES provisioner_jobs (id) ON DELETE CASCADE,
     PRIMARY KEY (id),
-    UNIQUE(workspace_id, name)
+    UNIQUE(workspace_id, name),
+    UNIQUE(workspace_id, build_number)
 );
diff --git a/coderd/database/models.go b/coderd/database/models.go
diff --git a/coderd/database/postgres/postgres_test.go b/coderd/database/postgres/postgres_test.go
@@ -17,8 +17,10 @@ func TestMain(m *testing.M) {
 	goleak.VerifyTestMain(m)
 }
 
+// nolint:paralleltest
 func TestPostgres(t *testing.T) {
-	t.Parallel()
+	// postgres.Open() seems to be creating race conditions when run in parallel.
+	// t.Parallel()
 
 	if testing.Short() {
 		t.Skip()

diff --git a/coderd/database/pubsub_test.go b/coderd/database/pubsub_test.go
@@ -22,8 +22,10 @@ func TestPubsub(t *testing.T) {
 		return
 	}
 
+	// nolint:paralleltest
 	t.Run("Postgres", func(t *testing.T) {
-		t.Parallel()
+		// postgres.Open() seems to be creating race conditions when run in parallel.
+		// t.Parallel()
 		ctx, cancelFunc := context.WithCancel(context.Background())
 		defer cancelFunc()
 
@@ -52,8 +54,10 @@ func TestPubsub(t *testing.T) {
 		assert.Equal(t, string(message), data)
 	})
 
+	// nolint:paralleltest
 	t.Run("PostgresCloseCancel", func(t *testing.T) {
-		t.Parallel()
+		// postgres.Open() seems to be creating race conditions when run in parallel.
+		// t.Parallel()
 		ctx, cancelFunc := context.WithCancel(context.Background())
 		defer cancelFunc()
 		connectionURL, closePg, err := postgres.Open()

diff --git a/coderd/database/querier.go b/coderd/database/querier.go