coder · deansheather · Jan 26, 2024 · Dec 18, 2023 · Dec 19, 2023 · Dec 19, 2023
diff --git a/coderd/agentapi/activitybump.go b/coderd/agentapi/activitybump.go
@@ -41,13 +41,14 @@ func ActivityBumpWorkspace(ctx context.Context, log slog.Logger, db database.Sto
 	// low priority operations fail first.
 	ctx, cancel := context.WithTimeout(ctx, time.Second*15)
 	defer cancel()
-	if err := db.ActivityBumpWorkspace(ctx, database.ActivityBumpWorkspaceParams{
+	err := db.ActivityBumpWorkspace(ctx, database.ActivityBumpWorkspaceParams{
 		NextAutostart: nextAutostart.UTC(),
 		WorkspaceID:   workspaceID,
-	}); err != nil {
+	})
+	if err != nil {
 		if !xerrors.Is(err, context.Canceled) && !database.IsQueryCanceledError(err) {
 			// Bump will fail if the context is canceled, but this is ok.
-			log.Error(ctx, "bump failed", slog.Error(err),
+			log.Error(ctx, "activity bump failed", slog.Error(err),
 				slog.F("workspace_id", workspaceID),
 			)
 		}

diff --git a/coderd/agentapi/api.go b/coderd/agentapi/api.go
@@ -17,7 +17,6 @@ import (
 
 	"cdr.dev/slog"
 	agentproto "github.com/coder/coder/v2/agent/proto"
-	"github.com/coder/coder/v2/coderd/batchstats"
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/pubsub"
 	"github.com/coder/coder/v2/coderd/externalauth"
@@ -61,19 +60,17 @@ type Options struct {
 	DerpMapFn                         func() *tailcfg.DERPMap
 	TailnetCoordinator                *atomic.Pointer[tailnet.Coordinator]
 	TemplateScheduleStore             *atomic.Pointer[schedule.TemplateScheduleStore]
-	StatsBatcher                      *batchstats.Batcher
+	StatsBatcher                      StatsBatcher
 	PublishWorkspaceUpdateFn          func(ctx context.Context, workspaceID uuid.UUID)
 	PublishWorkspaceAgentLogsUpdateFn func(ctx context.Context, workspaceAgentID uuid.UUID, msg agentsdk.LogsNotifyMessage)
 
-	AccessURL                       *url.URL
-	AppHostname                     string
-	AgentInactiveDisconnectTimeout  time.Duration
-	AgentFallbackTroubleshootingURL string
-	AgentStatsRefreshInterval       time.Duration
-	DisableDirectConnections        bool
-	DerpForceWebSockets             bool
-	DerpMapUpdateFrequency          time.Duration
-	ExternalAuthConfigs             []*externalauth.Config
+	AccessURL                 *url.URL
+	AppHostname               string
+	AgentStatsRefreshInterval time.Duration
+	DisableDirectConnections  bool
+	DerpForceWebSockets       bool
+	DerpMapUpdateFrequency    time.Duration
+	ExternalAuthConfigs       []*externalauth.Config
 
 	// Optional:
 	// WorkspaceID avoids a future lookup to find the workspace ID by setting
@@ -90,17 +87,14 @@ func New(opts Options) *API {
 	}
 
 	api.ManifestAPI = &ManifestAPI{
-		AccessURL:                       opts.AccessURL,
-		AppHostname:                     opts.AppHostname,
-		AgentInactiveDisconnectTimeout:  opts.AgentInactiveDisconnectTimeout,
-		AgentFallbackTroubleshootingURL: opts.AgentFallbackTroubleshootingURL,
-		ExternalAuthConfigs:             opts.ExternalAuthConfigs,
-		DisableDirectConnections:        opts.DisableDirectConnections,
-		DerpForceWebSockets:             opts.DerpForceWebSockets,
-		AgentFn:                         api.agent,
-		Database:                        opts.Database,
-		DerpMapFn:                       opts.DerpMapFn,
-		TailnetCoordinator:              opts.TailnetCoordinator,
+		AccessURL:                opts.AccessURL,
+		AppHostname:              opts.AppHostname,
+		ExternalAuthConfigs:      opts.ExternalAuthConfigs,
+		DisableDirectConnections: opts.DisableDirectConnections,
+		DerpForceWebSockets:      opts.DerpForceWebSockets,
+		AgentFn:                  api.agent,
+		Database:                 opts.Database,
+		DerpMapFn:                opts.DerpMapFn,
 	}
 
 	api.ServiceBannerAPI = &ServiceBannerAPI{
@@ -214,20 +208,15 @@ func (a *API) workspaceID(ctx context.Context, agent *database.WorkspaceAgent) (
 		agent = &agnt
 	}
 
-	resource, err := a.opts.Database.GetWorkspaceResourceByID(ctx, agent.ResourceID)
-	if err != nil {
-		return uuid.Nil, xerrors.Errorf("get workspace agent resource by id %q: %w", agent.ResourceID, err)
-	}
-
-	build, err := a.opts.Database.GetWorkspaceBuildByJobID(ctx, resource.JobID)
+	getWorkspaceAgentByIDRow, err := a.opts.Database.GetWorkspaceByAgentID(ctx, agent.ID)
 	if err != nil {
-		return uuid.Nil, xerrors.Errorf("get workspace build by job id %q: %w", resource.JobID, err)
+		return uuid.Nil, xerrors.Errorf("get workspace by agent id %q: %w", agent.ID, err)
 	}
 
 	a.mu.Lock()
-	a.cachedWorkspaceID = build.WorkspaceID
+	a.cachedWorkspaceID = getWorkspaceAgentByIDRow.Workspace.ID
 	a.mu.Unlock()
-	return build.WorkspaceID, nil
+	return getWorkspaceAgentByIDRow.Workspace.ID, nil
 }
 
 func (a *API) publishWorkspaceUpdate(ctx context.Context, agent *database.WorkspaceAgent) error {

diff --git a/coderd/agentapi/apps.go b/coderd/agentapi/apps.go
@@ -90,9 +90,11 @@ func (a *AppsAPI) BatchUpdateAppHealths(ctx context.Context, req *agentproto.Bat
 		}
 	}
 
-	err = a.PublishWorkspaceUpdateFn(ctx, &workspaceAgent)
-	if err != nil {
-		return nil, xerrors.Errorf("publish workspace update: %w", err)
+	if a.PublishWorkspaceUpdateFn != nil && len(newApps) > 0 {
+		err = a.PublishWorkspaceUpdateFn(ctx, &workspaceAgent)
+		if err != nil {
+			return nil, xerrors.Errorf("publish workspace update: %w", err)
+		}
 	}
 	return &agentproto.BatchUpdateAppHealthResponse{}, nil
 }
diff --git a/coderd/agentapi/apps_test.go b/coderd/agentapi/apps_test.go
@@ -0,0 +1,252 @@
+package agentapi_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
+
+	"cdr.dev/slog/sloggers/slogtest"
+
+	agentproto "github.com/coder/coder/v2/agent/proto"
+	"github.com/coder/coder/v2/coderd/agentapi"
+	"github.com/coder/coder/v2/coderd/database"
+	"github.com/coder/coder/v2/coderd/database/dbmock"
+)
+
+func TestBatchUpdateAppHealths(t *testing.T) {
+	t.Parallel()
+
+	var (
+		agent = database.WorkspaceAgent{
+			ID: uuid.New(),
+		}
+		app1 = database.WorkspaceApp{
+			ID:             uuid.New(),
+			AgentID:        agent.ID,
+			Slug:           "code-server-1",
+			DisplayName:    "code-server 1",
+			HealthcheckUrl: "http://localhost:3000",
+			Health:         database.WorkspaceAppHealthInitializing,
+		}
+		app2 = database.WorkspaceApp{
+			ID:             uuid.New(),
+			AgentID:        agent.ID,
+			Slug:           "code-server-2",
+			DisplayName:    "code-server 2",
+			HealthcheckUrl: "http://localhost:3001",
+			Health:         database.WorkspaceAppHealthHealthy,
+		}
+	)
+
+	t.Run("OK", func(t *testing.T) {
+		t.Parallel()
+
+		dbM := dbmock.NewMockStore(gomock.NewController(t))
+		dbM.EXPECT().GetWorkspaceAppsByAgentID(gomock.Any(), agent.ID).Return([]database.WorkspaceApp{app1, app2}, nil)
+		dbM.EXPECT().UpdateWorkspaceAppHealthByID(gomock.Any(), database.UpdateWorkspaceAppHealthByIDParams{
+			ID:     app1.ID,
+			Health: database.WorkspaceAppHealthHealthy,
+		}).Return(nil)
+		dbM.EXPECT().UpdateWorkspaceAppHealthByID(gomock.Any(), database.UpdateWorkspaceAppHealthByIDParams{
+			ID:     app2.ID,
+			Health: database.WorkspaceAppHealthUnhealthy,
+		}).Return(nil)
+
+		publishCalled := false
+		api := &agentapi.AppsAPI{
+			AgentFn: func(context.Context) (database.WorkspaceAgent, error) {
+				return agent, nil
+			},
+			Database: dbM,
+			Log:      slogtest.Make(t, nil),
+			PublishWorkspaceUpdateFn: func(ctx context.Context, wa *database.WorkspaceAgent) error {
+				publishCalled = true
+				return nil
+			},
+		}
+
+		// Set one to healthy, set another to unhealthy.
+		resp, err := api.BatchUpdateAppHealths(context.Background(), &agentproto.BatchUpdateAppHealthRequest{
+			Updates: []*agentproto.BatchUpdateAppHealthRequest_HealthUpdate{
+				{
+					Id:     app1.ID[:],
+					Health: agentproto.AppHealth_HEALTHY,
+				},
+				{
+					Id:     app2.ID[:],
+					Health: agentproto.AppHealth_UNHEALTHY,
+				},
+			},
+		})
+		require.NoError(t, err)
+		require.Equal(t, &agentproto.BatchUpdateAppHealthResponse{}, resp)
+
+		require.True(t, publishCalled)
+	})
+
+	t.Run("Unchanged", func(t *testing.T) {
+		t.Parallel()
+
+		dbM := dbmock.NewMockStore(gomock.NewController(t))
+		dbM.EXPECT().GetWorkspaceAppsByAgentID(gomock.Any(), agent.ID).Return([]database.WorkspaceApp{app1, app2}, nil)
+
+		publishCalled := false
+		api := &agentapi.AppsAPI{
+			AgentFn: func(context.Context) (database.WorkspaceAgent, error) {
+				return agent, nil
+			},
+			Database: dbM,
+			Log:      slogtest.Make(t, nil),
+			PublishWorkspaceUpdateFn: func(ctx context.Context, wa *database.WorkspaceAgent) error {
+				publishCalled = true
+				return nil
+			},
+		}
+
+		// Set both to their current status, neither should be updated in the
+		// DB.
+		resp, err := api.BatchUpdateAppHealths(context.Background(), &agentproto.BatchUpdateAppHealthRequest{
+			Updates: []*agentproto.BatchUpdateAppHealthRequest_HealthUpdate{
+				{
+					Id:     app1.ID[:],
+					Health: agentproto.AppHealth_INITIALIZING,
+				},
+				{
+					Id:     app2.ID[:],
+					Health: agentproto.AppHealth_HEALTHY,
+				},
+			},
+		})
+		require.NoError(t, err)
+		require.Equal(t, &agentproto.BatchUpdateAppHealthResponse{}, resp)
+
+		require.False(t, publishCalled)
+	})
+
+	t.Run("Empty", func(t *testing.T) {
+		t.Parallel()
+
+		// No DB queries are made if there are no updates to process.
+		dbM := dbmock.NewMockStore(gomock.NewController(t))
+
+		publishCalled := false
+		api := &agentapi.AppsAPI{
+			AgentFn: func(context.Context) (database.WorkspaceAgent, error) {
+				return agent, nil
+			},
+			Database: dbM,
+			Log:      slogtest.Make(t, nil),
+			PublishWorkspaceUpdateFn: func(ctx context.Context, wa *database.WorkspaceAgent) error {
+				publishCalled = true
+				return nil
+			},
+		}
+
+		// Do nothing.
+		resp, err := api.BatchUpdateAppHealths(context.Background(), &agentproto.BatchUpdateAppHealthRequest{
+			Updates: []*agentproto.BatchUpdateAppHealthRequest_HealthUpdate{},
+		})
+		require.NoError(t, err)
+		require.Equal(t, &agentproto.BatchUpdateAppHealthResponse{}, resp)
+
+		require.False(t, publishCalled)
+	})
+
+	t.Run("AppNoHealthcheck", func(t *testing.T) {
+		t.Parallel()
+
+		app3 := database.WorkspaceApp{
+			ID:          uuid.New(),
+			AgentID:     agent.ID,
+			Slug:        "code-server-3",
+			DisplayName: "code-server 3",
+		}
+
+		dbM := dbmock.NewMockStore(gomock.NewController(t))
+		dbM.EXPECT().GetWorkspaceAppsByAgentID(gomock.Any(), agent.ID).Return([]database.WorkspaceApp{app3}, nil)
+
+		api := &agentapi.AppsAPI{
+			AgentFn: func(context.Context) (database.WorkspaceAgent, error) {
+				return agent, nil
+			},
+			Database:                 dbM,
+			Log:                      slogtest.Make(t, nil),
+			PublishWorkspaceUpdateFn: nil,
+		}
+
+		// Set app3 to healthy, should error.
+		resp, err := api.BatchUpdateAppHealths(context.Background(), &agentproto.BatchUpdateAppHealthRequest{
+			Updates: []*agentproto.BatchUpdateAppHealthRequest_HealthUpdate{
+				{
+					Id:     app3.ID[:],
+					Health: agentproto.AppHealth_HEALTHY,
+				},
+			},
+		})
+		require.Error(t, err)
+		require.ErrorContains(t, err, "does not have healthchecks enabled")
+		require.Nil(t, resp)
+	})
+
+	t.Run("UnknownApp", func(t *testing.T) {
+		t.Parallel()
+
+		dbM := dbmock.NewMockStore(gomock.NewController(t))
+		dbM.EXPECT().GetWorkspaceAppsByAgentID(gomock.Any(), agent.ID).Return([]database.WorkspaceApp{app1, app2}, nil)
+
+		api := &agentapi.AppsAPI{
+			AgentFn: func(context.Context) (database.WorkspaceAgent, error) {
+				return agent, nil
+			},
+			Database:                 dbM,
+			Log:                      slogtest.Make(t, nil),
+			PublishWorkspaceUpdateFn: nil,
+		}
+
+		// Set an unknown app to healthy, should error.
+		id := uuid.New()
+		resp, err := api.BatchUpdateAppHealths(context.Background(), &agentproto.BatchUpdateAppHealthRequest{
+			Updates: []*agentproto.BatchUpdateAppHealthRequest_HealthUpdate{
+				{
+					Id:     id[:],
+					Health: agentproto.AppHealth_HEALTHY,
+				},
+			},
+		})
+		require.Error(t, err)
+		require.ErrorContains(t, err, "not found")
+		require.Nil(t, resp)
+	})
+
+	t.Run("InvalidHealth", func(t *testing.T) {
+		t.Parallel()
+
+		dbM := dbmock.NewMockStore(gomock.NewController(t))
+		dbM.EXPECT().GetWorkspaceAppsByAgentID(gomock.Any(), agent.ID).Return([]database.WorkspaceApp{app1, app2}, nil)
+
+		api := &agentapi.AppsAPI{
+			AgentFn: func(context.Context) (database.WorkspaceAgent, error) {
+				return agent, nil
+			},
+			Database:                 dbM,
+			Log:                      slogtest.Make(t, nil),
+			PublishWorkspaceUpdateFn: nil,
+		}
+
+		// Set an unknown app to healthy, should error.
+		resp, err := api.BatchUpdateAppHealths(context.Background(), &agentproto.BatchUpdateAppHealthRequest{
+			Updates: []*agentproto.BatchUpdateAppHealthRequest_HealthUpdate{
+				{
+					Id:     app1.ID[:],
+					Health: -999,
+				},
+			},
+		})
+		require.Error(t, err)
+		require.ErrorContains(t, err, "unknown health status")
+		require.Nil(t, resp)
+	})
+}