feat: order workspaces by running first (#7656)

* wip

* use updated sql

* wip

* Implement sorting in databasefake.go

* More fixes

* sql fmt

---------

Co-authored-by: Marcin Tojek <marcin@coder.com>

coderd/database/dbfake/databasefake.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"database/sql"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"reflect"
 	"regexp"
@@ -1329,6 +1330,63 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
 		workspaces = append(workspaces, workspace)
 	}
 
+	// Sort workspaces (ORDER BY)
+	isRunning := func(build database.WorkspaceBuild, job database.ProvisionerJob) bool {
+		return job.CompletedAt.Valid && !job.CanceledAt.Valid && !job.Error.Valid && build.Transition == database.WorkspaceTransitionStart
+	}
+
+	preloadedWorkspaceBuilds := map[uuid.UUID]database.WorkspaceBuild{}
+	preloadedProvisionerJobs := map[uuid.UUID]database.ProvisionerJob{}
+	preloadedUsers := map[uuid.UUID]database.User{}
+
+	for _, w := range workspaces {
+		build, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, w.ID)
+		if err == nil {
+			preloadedWorkspaceBuilds[w.ID] = build
+		} else if !errors.Is(err, sql.ErrNoRows) {
+			return nil, xerrors.Errorf("get latest build: %w", err)
+		}
+
+		job, err := q.getProvisionerJobByIDNoLock(ctx, build.JobID)
+		if err == nil {
+			preloadedProvisionerJobs[w.ID] = job
+		} else if !errors.Is(err, sql.ErrNoRows) {
+			return nil, xerrors.Errorf("get provisioner job: %w", err)
+		}
+
+		user, err := q.getUserByIDNoLock(w.OwnerID)
+		if err == nil {
+			preloadedUsers[w.ID] = user
+		} else if !errors.Is(err, sql.ErrNoRows) {
+			return nil, xerrors.Errorf("get user: %w", err)
+		}
+	}
+
+	sort.Slice(workspaces, func(i, j int) bool {
+		w1 := workspaces[i]
+		w2 := workspaces[j]
+
+		// Order by: running first
+		w1IsRunning := isRunning(preloadedWorkspaceBuilds[w1.ID], preloadedProvisionerJobs[w1.ID])
+		w2IsRunning := isRunning(preloadedWorkspaceBuilds[w2.ID], preloadedProvisionerJobs[w2.ID])
+
+		if w1IsRunning && !w2IsRunning {
+			return true
+		}
+
+		if !w1IsRunning && w2IsRunning {
+			return false
+		}
+
+		// Order by: usernames
+		if w1.ID != w2.ID {
+			return sort.StringsAreSorted([]string{preloadedUsers[w1.ID].Username, preloadedUsers[w2.ID].Username})
+		}
+
+		// Order by: workspace names
+		return sort.StringsAreSorted([]string{w1.Name, w2.Name})
+	})
+
 	beforePageCount := len(workspaces)
 
 	if arg.Offset > 0 {

coderd/database/queries.sql.go

@@ -8172,7 +8172,11 @@ const getWorkspaces = `-- name: GetWorkspaces :many
 SELECT
 	workspaces.id, workspaces.created_at, workspaces.updated_at, workspaces.owner_id, workspaces.organization_id, workspaces.template_id, workspaces.deleted, workspaces.name, workspaces.autostart_schedule, workspaces.ttl, workspaces.last_used_at, COUNT(*) OVER () as count
 FROM
-	workspaces
+    workspaces
+JOIN
+    users
+ON
+    workspaces.owner_id = users.id
 LEFT JOIN LATERAL (
 	SELECT
 		workspace_builds.transition,
@@ -8333,7 +8337,12 @@ WHERE
 	-- Authorize Filter clause will be injected below in GetAuthorizedWorkspaces
 	-- @authorize_filter
 ORDER BY
-	last_used_at DESC
+	(latest_build.completed_at IS NOT NULL AND
+		latest_build.canceled_at IS NULL AND
+		latest_build.error IS NULL AND
+		latest_build.transition = 'start'::workspace_transition) DESC,
+	LOWER(users.username) ASC,
+	LOWER(name) ASC
 LIMIT
 	CASE
 		WHEN $11 :: integer > 0 THEN

coderd/database/queries/workspaces.sql

@@ -77,7 +77,11 @@ WHERE
 SELECT
 	workspaces.*, COUNT(*) OVER () as count
 FROM
-	workspaces
+    workspaces
+JOIN
+    users
+ON
+    workspaces.owner_id = users.id
 LEFT JOIN LATERAL (
 	SELECT
 		workspace_builds.transition,
@@ -238,7 +242,12 @@ WHERE
 	-- Authorize Filter clause will be injected below in GetAuthorizedWorkspaces
 	-- @authorize_filter
 ORDER BY
-	last_used_at DESC
+	(latest_build.completed_at IS NOT NULL AND
+		latest_build.canceled_at IS NULL AND
+		latest_build.error IS NULL AND
+		latest_build.transition = 'start'::workspace_transition) DESC,
+	LOWER(users.username) ASC,
+	LOWER(name) ASC
 LIMIT
 	CASE
 		WHEN @limit_ :: integer > 0 THEN

coderd/workspaces.go

@@ -7,7 +7,6 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"sort"
 	"strconv"
 	"time"
 
@@ -1015,36 +1014,9 @@ func convertWorkspaces(workspaces []database.Workspace, data workspaceData) ([]c
 			&owner,
 		))
 	}
-
-	sortWorkspaces(apiWorkspaces)
-
 	return apiWorkspaces, nil
 }
 
-func sortWorkspaces(workspaces []codersdk.Workspace) {
-	sort.Slice(workspaces, func(i, j int) bool {
-		iw := workspaces[i]
-		jw := workspaces[j]
-
-		if iw.LatestBuild.Status == codersdk.WorkspaceStatusRunning && jw.LatestBuild.Status != codersdk.WorkspaceStatusRunning {
-			return true
-		}
-
-		if jw.LatestBuild.Status == codersdk.WorkspaceStatusRunning && iw.LatestBuild.Status != codersdk.WorkspaceStatusRunning {
-			return false
-		}
-
-		if iw.OwnerID != jw.OwnerID {
-			return iw.OwnerName < jw.OwnerName
-		}
-
-		if jw.LastUsedAt.IsZero() && iw.LastUsedAt.IsZero() {
-			return iw.Name < jw.Name
-		}
-		return iw.LastUsedAt.After(jw.LastUsedAt)
-	})
-}
-
 func convertWorkspace(
 	workspace database.Workspace,
 	workspaceBuild codersdk.WorkspaceBuild,

coderd/workspaces_internal_test.go

@@ -4,7 +4,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/coderd/database"
@@ -81,96 +80,3 @@ func Test_calculateDeletingAt(t *testing.T) {
 		})
 	}
 }
-
-func TestSortWorkspaces(t *testing.T) {
-	// the correct sorting order is:
-	// 1. first show workspaces that are currently running,
-	// 2. then sort by user_name,
-	// 3. then sort by last_used_at (descending),
-	t.Parallel()
-
-	workspaceFactory := func(t *testing.T, name string, ownerID uuid.UUID, ownerName string, status codersdk.WorkspaceStatus, lastUsedAt time.Time) codersdk.Workspace {
-		t.Helper()
-		return codersdk.Workspace{
-			ID:        uuid.New(),
-			OwnerID:   ownerID,
-			OwnerName: ownerName,
-			LatestBuild: codersdk.WorkspaceBuild{
-				Status: status,
-			},
-			Name:       name,
-			LastUsedAt: lastUsedAt,
-		}
-	}
-
-	userAuuid := uuid.New()
-
-	workspaceRunningUserA := workspaceFactory(t, "running-userA", userAuuid, "userA", codersdk.WorkspaceStatusRunning, time.Now())
-	workspaceRunningUserB := workspaceFactory(t, "running-userB", uuid.New(), "userB", codersdk.WorkspaceStatusRunning, time.Now())
-	workspacePendingUserC := workspaceFactory(t, "pending-userC", uuid.New(), "userC", codersdk.WorkspaceStatusPending, time.Now())
-	workspaceRunningUserA2 := workspaceFactory(t, "running-userA2", userAuuid, "userA", codersdk.WorkspaceStatusRunning, time.Now().Add(time.Minute))
-	workspaceRunningUserZ := workspaceFactory(t, "running-userZ", uuid.New(), "userZ", codersdk.WorkspaceStatusRunning, time.Now())
-	workspaceRunningUserA3 := workspaceFactory(t, "running-userA3", userAuuid, "userA", codersdk.WorkspaceStatusRunning, time.Now().Add(time.Hour))
-
-	testCases := []struct {
-		name          string
-		input         []codersdk.Workspace
-		expectedOrder []string
-	}{
-		{
-			name: "Running workspaces should be first",
-			input: []codersdk.Workspace{
-				workspaceRunningUserB,
-				workspacePendingUserC,
-				workspaceRunningUserA,
-			},
-			expectedOrder: []string{
-				"running-userA",
-				"running-userB",
-				"pending-userC",
-			},
-		},
-		{
-			name: "then sort by owner name",
-			input: []codersdk.Workspace{
-				workspaceRunningUserZ,
-				workspaceRunningUserA,
-				workspaceRunningUserB,
-			},
-			expectedOrder: []string{
-				"running-userA",
-				"running-userB",
-				"running-userZ",
-			},
-		},
-		{
-			name: "then sort by last used at (recent first)",
-			input: []codersdk.Workspace{
-				workspaceRunningUserA,
-				workspaceRunningUserA2,
-				workspaceRunningUserA3,
-			},
-			expectedOrder: []string{
-				"running-userA3",
-				"running-userA2",
-				"running-userA",
-			},
-		},
-	}
-
-	for _, tc := range testCases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-			workspaces := tc.input
-			sortWorkspaces(workspaces)
-
-			var resultNames []string
-			for _, workspace := range workspaces {
-				resultNames = append(resultNames, workspace.Name)
-			}
-
-			require.Equal(t, tc.expectedOrder, resultNames, tc.name)
-		})
-	}
-}

coderd/workspaces_test.go

@@ -198,6 +198,60 @@ func TestAdminViewAllWorkspaces(t *testing.T) {
 	require.Equal(t, 0, len(memberViewWorkspaces.Workspaces), "member in other org should see 0 workspaces")
 }
 
+func TestWorkspacesSortOrder(t *testing.T) {
+	t.Parallel()
+
+	client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
+	firstUser := coderdtest.CreateFirstUser(t, client)
+	version := coderdtest.CreateTemplateVersion(t, client, firstUser.OrganizationID, nil)
+	coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
+	template := coderdtest.CreateTemplate(t, client, firstUser.OrganizationID, version.ID)
+
+	// c-workspace should be running
+	workspace1 := coderdtest.CreateWorkspace(t, client, firstUser.OrganizationID, template.ID, func(ctr *codersdk.CreateWorkspaceRequest) {
+		ctr.Name = "c-workspace"
+	})
+	coderdtest.AwaitWorkspaceBuildJob(t, client, workspace1.LatestBuild.ID)
+
+	// b-workspace should be stopped
+	workspace2 := coderdtest.CreateWorkspace(t, client, firstUser.OrganizationID, template.ID, func(ctr *codersdk.CreateWorkspaceRequest) {
+		ctr.Name = "b-workspace"
+	})
+	coderdtest.AwaitWorkspaceBuildJob(t, client, workspace2.LatestBuild.ID)
+
+	build2 := coderdtest.CreateWorkspaceBuild(t, client, workspace2, database.WorkspaceTransitionStop)
+	coderdtest.AwaitWorkspaceBuildJob(t, client, build2.ID)
+
+	// a-workspace should be running
+	workspace3 := coderdtest.CreateWorkspace(t, client, firstUser.OrganizationID, template.ID, func(ctr *codersdk.CreateWorkspaceRequest) {
+		ctr.Name = "a-workspace"
+	})
+	coderdtest.AwaitWorkspaceBuildJob(t, client, workspace3.LatestBuild.ID)
+
+	ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+	defer cancel()
+	workspacesResponse, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{})
+	require.NoError(t, err, "(first) fetch workspaces")
+	workspaces := workspacesResponse.Workspaces
+
+	expected := []string{
+		workspace3.Name,
+		workspace1.Name,
+		workspace2.Name,
+	}
+
+	var actual []string
+	for _, w := range workspaces {
+		actual = append(actual, w.Name)
+	}
+
+	// the correct sorting order is:
+	// 1. Running workspaces
+	// 2. Sort by usernames
+	// 3. Sort by workspace names
+	require.Equal(t, expected, actual)
+}
+
 func TestPostWorkspacesByOrganization(t *testing.T) {
 	t.Parallel()
 	t.Run("InvalidTemplate", func(t *testing.T) {