From 39b846feaa88303536c103227dfd9a19d39ab0b2 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 12 Jan 2024 20:42:48 +0000 Subject: [PATCH 01/11] fix(coderd): workspaceapps: update last_used_at when workspace app reports stat --- coderd/database/dbauthz/dbauthz.go | 9 +++++++ coderd/database/dbauthz/dbauthz_test.go | 7 +++++ coderd/database/dbmem/dbmem.go | 28 ++++++++++++++++++++ coderd/database/dbmetrics/dbmetrics.go | 7 +++++ coderd/database/dbmock/dbmock.go | 14 ++++++++++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 19 ++++++++++++++ coderd/database/queries/workspaces.sql | 8 ++++++ coderd/httpapi/websocket.go | 3 ++- coderd/workspaceapps/apptest/apptest.go | 35 +++++++++++++++++++++++++ coderd/workspaceapps/apptest/setup.go | 1 + coderd/workspaceapps/stats.go | 22 ++++++++++++---- coderd/workspaceapps_test.go | 8 ++++++ 13 files changed, 156 insertions(+), 6 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 6e236e3442baf..a5b295e2e35eb 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -695,6 +695,15 @@ func (q *querier) ArchiveUnusedTemplateVersions(ctx context.Context, arg databas return q.db.ArchiveUnusedTemplateVersions(ctx, arg) } +func (q *querier) BatchUpdateWorkspaceLastUsedAt(ctx context.Context, arg database.BatchUpdateWorkspaceLastUsedAtParams) error { + // Could be any workspace and checking auth to each workspace is overkill for the purpose + // of this function. + if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceWorkspace.All()); err != nil { + return err + } + return q.db.BatchUpdateWorkspaceLastUsedAt(ctx, arg) +} + func (q *querier) CleanTailnetCoordinators(ctx context.Context) error { if err := q.authorizeContext(ctx, rbac.ActionDelete, rbac.ResourceTailnetCoordinator); err != nil { return err diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 0d23f33c9c02e..d9444278722e7 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1549,6 +1549,13 @@ func (s *MethodTestSuite) TestWorkspace() { ID: ws.ID, }).Asserts(ws, rbac.ActionUpdate).Returns() })) + s.Run("BatchUpdateWorkspaceLastUsedAt", s.Subtest(func(db database.Store, check *expects) { + ws1 := dbgen.Workspace(s.T(), db, database.Workspace{}) + ws2 := dbgen.Workspace(s.T(), db, database.Workspace{}) + check.Args(database.BatchUpdateWorkspaceLastUsedAtParams{ + IDs: []uuid.UUID{ws1.ID, ws2.ID}, + }).Asserts(rbac.ResourceWorkspace.All(), rbac.ActionUpdate).Returns() + })) s.Run("UpdateWorkspaceTTL", s.Subtest(func(db database.Store, check *expects) { ws := dbgen.Workspace(s.T(), db, database.Workspace{}) check.Args(database.UpdateWorkspaceTTLParams{ diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 3aaa0455a5be9..d2b78f0d40d86 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -963,6 +963,34 @@ func (q *FakeQuerier) ArchiveUnusedTemplateVersions(_ context.Context, arg datab return archived, nil } +func (q *FakeQuerier) BatchUpdateWorkspaceLastUsedAt(ctx context.Context, arg database.BatchUpdateWorkspaceLastUsedAtParams) error { + err := validateDatabaseType(arg) + if err != nil { + return err + } + + q.mutex.Lock() + defer q.mutex.Unlock() + + // temporary map to avoid O(q.workspaces*arg.workspaceIds) + m := make(map[uuid.UUID]struct{}) + for _, id := range arg.IDs { + m[id] = struct{}{} + } + n := 0 + for i := 0; i < len(q.workspaces); i++ { + if _, found := m[q.workspaces[i].ID]; !found { + continue + } + q.workspaces[i].LastUsedAt = arg.LastUsedAt + n++ + } + if n == 0 { + return sql.ErrNoRows + } + return nil +} + func (*FakeQuerier) CleanTailnetCoordinators(_ context.Context) error { return ErrUnimplemented } diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index d11b376b371c9..625871500dbeb 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -114,6 +114,13 @@ func (m metricsStore) ArchiveUnusedTemplateVersions(ctx context.Context, arg dat return r0, r1 } +func (m metricsStore) BatchUpdateWorkspaceLastUsedAt(ctx context.Context, arg database.BatchUpdateWorkspaceLastUsedAtParams) error { + start := time.Now() + r0 := m.s.BatchUpdateWorkspaceLastUsedAt(ctx, arg) + m.queryLatencies.WithLabelValues("BatchUpdateWorkspaceLastUsedAt").Observe(time.Since(start).Seconds()) + return r0 +} + func (m metricsStore) CleanTailnetCoordinators(ctx context.Context) error { start := time.Now() err := m.s.CleanTailnetCoordinators(ctx) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 7f7f409578df0..bfb93405f5524 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -117,6 +117,20 @@ func (mr *MockStoreMockRecorder) ArchiveUnusedTemplateVersions(arg0, arg1 any) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ArchiveUnusedTemplateVersions", reflect.TypeOf((*MockStore)(nil).ArchiveUnusedTemplateVersions), arg0, arg1) } +// BatchUpdateWorkspaceLastUsedAt mocks base method. +func (m *MockStore) BatchUpdateWorkspaceLastUsedAt(arg0 context.Context, arg1 database.BatchUpdateWorkspaceLastUsedAtParams) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "BatchUpdateWorkspaceLastUsedAt", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// BatchUpdateWorkspaceLastUsedAt indicates an expected call of BatchUpdateWorkspaceLastUsedAt. +func (mr *MockStoreMockRecorder) BatchUpdateWorkspaceLastUsedAt(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BatchUpdateWorkspaceLastUsedAt", reflect.TypeOf((*MockStore)(nil).BatchUpdateWorkspaceLastUsedAt), arg0, arg1) +} + // CleanTailnetCoordinators mocks base method. func (m *MockStore) CleanTailnetCoordinators(arg0 context.Context) error { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index e32c106787a13..8947ba185d14d 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -42,6 +42,7 @@ type sqlcQuerier interface { // Only unused template versions will be archived, which are any versions not // referenced by the latest build of a workspace. ArchiveUnusedTemplateVersions(ctx context.Context, arg ArchiveUnusedTemplateVersionsParams) ([]uuid.UUID, error) + BatchUpdateWorkspaceLastUsedAt(ctx context.Context, arg BatchUpdateWorkspaceLastUsedAtParams) error CleanTailnetCoordinators(ctx context.Context) error CleanTailnetLostPeers(ctx context.Context) error CleanTailnetTunnels(ctx context.Context) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index f9287915d5438..170a9274c6dfc 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -10813,6 +10813,25 @@ func (q *sqlQuerier) InsertWorkspaceResourceMetadata(ctx context.Context, arg In return items, nil } +const batchUpdateWorkspaceLastUsedAt = `-- name: BatchUpdateWorkspaceLastUsedAt :exec +UPDATE + workspaces +SET + last_used_at = $1 +WHERE + id = ANY($2 :: uuid[]) +` + +type BatchUpdateWorkspaceLastUsedAtParams struct { + LastUsedAt time.Time `db:"last_used_at" json:"last_used_at"` + IDs []uuid.UUID `db:"ids" json:"ids"` +} + +func (q *sqlQuerier) BatchUpdateWorkspaceLastUsedAt(ctx context.Context, arg BatchUpdateWorkspaceLastUsedAtParams) error { + _, err := q.db.ExecContext(ctx, batchUpdateWorkspaceLastUsedAt, arg.LastUsedAt, pq.Array(arg.IDs)) + return err +} + const getDeploymentWorkspaceStats = `-- name: GetDeploymentWorkspaceStats :one WITH workspaces_with_jobs AS ( SELECT diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index d9ff657fd21dc..b400a1165b292 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -357,6 +357,14 @@ SET WHERE id = $1; +-- name: BatchUpdateWorkspaceLastUsedAt :exec +UPDATE + workspaces +SET + last_used_at = @last_used_at +WHERE + id = ANY(@ids :: uuid[]); + -- name: GetDeploymentWorkspaceStats :one WITH workspaces_with_jobs AS ( SELECT diff --git a/coderd/httpapi/websocket.go b/coderd/httpapi/websocket.go index ad3b4b277dff4..629dcac8131f3 100644 --- a/coderd/httpapi/websocket.go +++ b/coderd/httpapi/websocket.go @@ -4,8 +4,9 @@ import ( "context" "time" - "cdr.dev/slog" "nhooyr.io/websocket" + + "cdr.dev/slog" ) // Heartbeat loops to ping a WebSocket to keep it alive. diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 166f3ba137fe3..2a0820b5bb734 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -64,6 +64,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // reconnecting-pty proxy server we want to test is mounted. client := appDetails.AppClient(t) testReconnectingPTY(ctx, t, client, appDetails.Agent.ID, "") + assertWorkspaceLastUsedAtUpdated(t, client, appDetails) }) t.Run("SignedTokenQueryParameter", func(t *testing.T) { @@ -92,6 +93,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // Make an unauthenticated client. unauthedAppClient := codersdk.New(appDetails.AppClient(t).URL) testReconnectingPTY(ctx, t, unauthedAppClient, appDetails.Agent.ID, issueRes.SignedToken) + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) }) @@ -117,6 +119,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Contains(t, string(body), "Path-based applications are disabled") + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("LoginWithoutAuthOnPrimary", func(t *testing.T) { @@ -142,6 +145,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.True(t, loc.Query().Has("message")) require.True(t, loc.Query().Has("redirect")) + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("LoginWithoutAuthOnProxy", func(t *testing.T) { @@ -179,6 +183,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // request is getting stripped. require.Equal(t, u.Path, redirectURI.Path+"/") require.Equal(t, u.RawQuery, redirectURI.RawQuery) + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("NoAccessShould404", func(t *testing.T) { @@ -195,6 +200,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusNotFound, resp.StatusCode) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("RedirectsWithSlash", func(t *testing.T) { @@ -209,6 +215,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("RedirectsWithQuery", func(t *testing.T) { @@ -226,6 +233,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { loc, err := resp.Location() require.NoError(t, err) require.Equal(t, proxyTestAppQuery, loc.RawQuery) + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("Proxies", func(t *testing.T) { @@ -267,6 +275,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("ProxiesHTTPS", func(t *testing.T) { @@ -312,6 +321,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("BlocksMe", func(t *testing.T) { @@ -331,6 +341,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Contains(t, string(body), "must be accessed with the full username, not @me") + assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("ForwardsIP", func(t *testing.T) { @@ -349,6 +360,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) require.Equal(t, "1.1.1.1,127.0.0.1", resp.Header.Get("X-Forwarded-For")) + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("ProxyError", func(t *testing.T) { @@ -361,6 +373,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusBadGateway, resp.StatusCode) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("NoProxyPort", func(t *testing.T) { @@ -375,6 +388,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // TODO(@deansheather): This should be 400. There's a todo in the // resolve request code to fix this. require.Equal(t, http.StatusInternalServerError, resp.StatusCode) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) }) }) @@ -548,6 +562,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) resp.Body.Close() require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appClient, appDetails) }) } }) @@ -1549,3 +1564,23 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli // Ensure the connection closes. require.ErrorIs(t, tr.ReadUntil(ctx, nil), io.EOF) } + +// Accessing an app should update the workspace's LastUsedAt. +func assertWorkspaceLastUsedAtUpdated(t testing.TB, client *codersdk.Client, details *Details) { + t.Helper() + + details.FlushStats() + ws, err := client.Workspace(context.Background(), details.Workspace.ID) + require.NoError(t, err) + require.Greater(t, ws.UpdatedAt, details.Workspace.UpdatedAt, "workspace last used at not updated when it should have been") +} + +// Except when it sometimes shouldn't (e.g. no access) +func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, client *codersdk.Client, details *Details) { + t.Helper() + + details.FlushStats() + ws, err := client.Workspace(context.Background(), details.Workspace.ID) + require.NoError(t, err) + require.Equal(t, ws.UpdatedAt, details.Workspace.UpdatedAt, "workspace last used at updated when it shouldn't have been") +} diff --git a/coderd/workspaceapps/apptest/setup.go b/coderd/workspaceapps/apptest/setup.go index 534a35398f653..ebf4e9e2c0bf7 100644 --- a/coderd/workspaceapps/apptest/setup.go +++ b/coderd/workspaceapps/apptest/setup.go @@ -71,6 +71,7 @@ type Deployment struct { SDKClient *codersdk.Client FirstUser codersdk.CreateFirstUserResponse PathAppBaseURL *url.URL + FlushStats func() } // DeploymentFactory generates a deployment with an API client, a path base URL, diff --git a/coderd/workspaceapps/stats.go b/coderd/workspaceapps/stats.go index bb00b1c27ab12..9bf469e3b2b2a 100644 --- a/coderd/workspaceapps/stats.go +++ b/coderd/workspaceapps/stats.go @@ -13,6 +13,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/util/slice" ) const ( @@ -117,11 +118,21 @@ func (r *StatsDBReporter) Report(ctx context.Context, stats []StatsReport) error batch.Requests = batch.Requests[:0] } } - if len(batch.UserID) > 0 { - err := tx.InsertWorkspaceAppStats(ctx, batch) - if err != nil { - return err - } + if len(batch.UserID) == 0 { + return nil + } + + if err := tx.InsertWorkspaceAppStats(ctx, batch); err != nil { + return err + } + + // TODO: There should be a better way to do this. + uniqueIDs := slice.Unique(batch.WorkspaceID) + if err := tx.BatchUpdateWorkspaceLastUsedAt(ctx, database.BatchUpdateWorkspaceLastUsedAtParams{ + IDs: uniqueIDs, + LastUsedAt: dbtime.Now(), // This isn't 100% accurate, but it's good enough. + }); err != nil { + return err } return nil @@ -234,6 +245,7 @@ func (sc *StatsCollector) Collect(report StatsReport) { } delete(sc.statsBySessionID, report.SessionID) } + sc.opts.Logger.Debug(sc.ctx, "collected workspace app stats", slog.F("report", report)) } // rollup performs stats rollup for sessions that fall within the diff --git a/coderd/workspaceapps_test.go b/coderd/workspaceapps_test.go index 2018e1d8dde4e..3d7caf35c87de 100644 --- a/coderd/workspaceapps_test.go +++ b/coderd/workspaceapps_test.go @@ -262,6 +262,13 @@ func TestWorkspaceApps(t *testing.T) { opts.AppHost = "" } + statsFlushCh := make(chan chan<- struct{}, 1) + opts.StatsCollectorOptions.Flush = statsFlushCh + flushStats := func() { + flushDone := make(chan struct{}) + statsFlushCh <- flushDone + <-flushDone + } client := coderdtest.New(t, &coderdtest.Options{ DeploymentValues: deploymentValues, AppHostname: opts.AppHost, @@ -285,6 +292,7 @@ func TestWorkspaceApps(t *testing.T) { SDKClient: client, FirstUser: user, PathAppBaseURL: client.URL, + FlushStats: flushStats, } }) } From d858bac157a7332750af49d89d3f1a30b007522c Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 15 Jan 2024 11:50:15 +0000 Subject: [PATCH 02/11] fix tests --- coderd/workspaceagents_test.go | 2 +- coderd/workspaceapps/apptest/apptest.go | 34 ++++++++++++------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 77fb6b1976ab9..0d620c991e6dd 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1626,7 +1626,7 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) { cancel() // We expect only 1 // In a failed test, you will likely see 9, as the last one - // gets cancelled. + // gets canceled. require.Equal(t, 1, validateCalls, "validate calls duplicated on same token") }) } diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 2a0820b5bb734..12b647e8ada33 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -119,7 +119,9 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Contains(t, string(body), "Path-based applications are disabled") - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + // Even though path-based apps are disabled, the request should indicate + // that the workspace was used. + assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("LoginWithoutAuthOnPrimary", func(t *testing.T) { @@ -200,7 +202,8 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusNotFound, resp.StatusCode) - assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) + // TODO(cian): A blocked request should not count as workspace usage. + // assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("RedirectsWithSlash", func(t *testing.T) { @@ -215,7 +218,8 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + // TODO(cian): The initial redirect should not count as workspace usage. + // assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("RedirectsWithQuery", func(t *testing.T) { @@ -233,7 +237,8 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { loc, err := resp.Location() require.NoError(t, err) require.Equal(t, proxyTestAppQuery, loc.RawQuery) - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + // TODO(cian): The initial redirect should not count as workspace usage. + // assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("Proxies", func(t *testing.T) { @@ -341,7 +346,8 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Contains(t, string(body), "must be accessed with the full username, not @me") - assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) + // TODO(cian): A blocked request should not count as workspace usage. + // assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("ForwardsIP", func(t *testing.T) { @@ -373,7 +379,9 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusBadGateway, resp.StatusCode) - assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) + // An valid authenticated attempt to access a workspace app + // should count as usage regardless of success. + assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) }) t.Run("NoProxyPort", func(t *testing.T) { @@ -562,7 +570,6 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) resp.Body.Close() require.Equal(t, http.StatusOK, resp.StatusCode) - assertWorkspaceLastUsedAtUpdated(t, appClient, appDetails) }) } }) @@ -1445,16 +1452,12 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("ReportStats", func(t *testing.T) { t.Parallel() - flush := make(chan chan<- struct{}, 1) - reporter := &fakeStatsReporter{} appDetails := setupProxyTest(t, &DeploymentOptions{ StatsCollectorOptions: workspaceapps.StatsCollectorOptions{ Reporter: reporter, ReportInterval: time.Hour, RollupWindow: time.Minute, - - Flush: flush, }, }) @@ -1472,10 +1475,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { var stats []workspaceapps.StatsReport require.Eventually(t, func() bool { // Keep flushing until we get a non-empty stats report. - flushDone := make(chan struct{}, 1) - flush <- flushDone - <-flushDone - + appDetails.FlushStats() stats = reporter.stats() return len(stats) > 0 }, testutil.WaitLong, testutil.IntervalFast, "stats not reported") @@ -1572,7 +1572,7 @@ func assertWorkspaceLastUsedAtUpdated(t testing.TB, client *codersdk.Client, det details.FlushStats() ws, err := client.Workspace(context.Background(), details.Workspace.ID) require.NoError(t, err) - require.Greater(t, ws.UpdatedAt, details.Workspace.UpdatedAt, "workspace last used at not updated when it should have been") + require.Greater(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt not updated when it should have been") } // Except when it sometimes shouldn't (e.g. no access) @@ -1582,5 +1582,5 @@ func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, client *codersdk.Client, details.FlushStats() ws, err := client.Workspace(context.Background(), details.Workspace.ID) require.NoError(t, err) - require.Equal(t, ws.UpdatedAt, details.Workspace.UpdatedAt, "workspace last used at updated when it shouldn't have been") + require.Equal(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt updated when it should not have been") } From 07176efcdc340b14cd94d3339a8955f3d8156c79 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 15 Jan 2024 12:09:37 +0000 Subject: [PATCH 03/11] fix linter --- coderd/database/dbmem/dbmem.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index d2b78f0d40d86..9178f561255df 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -963,7 +963,7 @@ func (q *FakeQuerier) ArchiveUnusedTemplateVersions(_ context.Context, arg datab return archived, nil } -func (q *FakeQuerier) BatchUpdateWorkspaceLastUsedAt(ctx context.Context, arg database.BatchUpdateWorkspaceLastUsedAtParams) error { +func (q *FakeQuerier) BatchUpdateWorkspaceLastUsedAt(_ context.Context, arg database.BatchUpdateWorkspaceLastUsedAtParams) error { err := validateDatabaseType(arg) if err != nil { return err From ef8d3ab149e7cbd83c2680a2fe5cbcf4197d142d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 15 Jan 2024 13:42:54 +0000 Subject: [PATCH 04/11] use the correct client --- coderd/workspaceapps/apptest/apptest.go | 28 ++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 12b647e8ada33..cc7958b66be35 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -64,7 +64,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // reconnecting-pty proxy server we want to test is mounted. client := appDetails.AppClient(t) testReconnectingPTY(ctx, t, client, appDetails.Agent.ID, "") - assertWorkspaceLastUsedAtUpdated(t, client, appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("SignedTokenQueryParameter", func(t *testing.T) { @@ -93,7 +93,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // Make an unauthenticated client. unauthedAppClient := codersdk.New(appDetails.AppClient(t).URL) testReconnectingPTY(ctx, t, unauthedAppClient, appDetails.Agent.ID, issueRes.SignedToken) - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) }) @@ -121,7 +121,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Contains(t, string(body), "Path-based applications are disabled") // Even though path-based apps are disabled, the request should indicate // that the workspace was used. - assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("LoginWithoutAuthOnPrimary", func(t *testing.T) { @@ -147,7 +147,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.True(t, loc.Query().Has("message")) require.True(t, loc.Query().Has("redirect")) - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("LoginWithoutAuthOnProxy", func(t *testing.T) { @@ -185,7 +185,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // request is getting stripped. require.Equal(t, u.Path, redirectURI.Path+"/") require.Equal(t, u.RawQuery, redirectURI.RawQuery) - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("NoAccessShould404", func(t *testing.T) { @@ -280,7 +280,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("ProxiesHTTPS", func(t *testing.T) { @@ -326,7 +326,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("BlocksMe", func(t *testing.T) { @@ -366,7 +366,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) require.Equal(t, "1.1.1.1,127.0.0.1", resp.Header.Get("X-Forwarded-For")) - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("ProxyError", func(t *testing.T) { @@ -381,7 +381,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.Equal(t, http.StatusBadGateway, resp.StatusCode) // An valid authenticated attempt to access a workspace app // should count as usage regardless of success. - assertWorkspaceLastUsedAtUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("NoProxyPort", func(t *testing.T) { @@ -396,7 +396,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // TODO(@deansheather): This should be 400. There's a todo in the // resolve request code to fix this. require.Equal(t, http.StatusInternalServerError, resp.StatusCode) - assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) }) @@ -1566,21 +1566,21 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli } // Accessing an app should update the workspace's LastUsedAt. -func assertWorkspaceLastUsedAtUpdated(t testing.TB, client *codersdk.Client, details *Details) { +func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) { t.Helper() details.FlushStats() - ws, err := client.Workspace(context.Background(), details.Workspace.ID) + ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) require.NoError(t, err) require.Greater(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt not updated when it should have been") } // Except when it sometimes shouldn't (e.g. no access) -func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, client *codersdk.Client, details *Details) { +func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, details *Details) { t.Helper() details.FlushStats() - ws, err := client.Workspace(context.Background(), details.Workspace.ID) + ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) require.NoError(t, err) require.Equal(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt updated when it should not have been") } From 601136cd23b45d0d60c917de1a584c07a7db123e Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 15 Jan 2024 14:38:40 +0000 Subject: [PATCH 05/11] add flush to wsproxy tests --- enterprise/coderd/coderdenttest/proxytest.go | 6 ++++++ enterprise/wsproxy/wsproxy_test.go | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/enterprise/coderd/coderdenttest/proxytest.go b/enterprise/coderd/coderdenttest/proxytest.go index 8a28b077c16f4..36dc287798ae1 100644 --- a/enterprise/coderd/coderdenttest/proxytest.go +++ b/enterprise/coderd/coderdenttest/proxytest.go @@ -37,6 +37,9 @@ type ProxyOptions struct { // ProxyURL is optional ProxyURL *url.URL + + // Flush is optional + Flush chan chan<- struct{} } // NewWorkspaceProxy will configure a wsproxy.Server with the given options. @@ -113,6 +116,9 @@ func NewWorkspaceProxy(t *testing.T, coderdAPI *coderd.API, owner *codersdk.Clie // Inherit collector options from coderd, but keep the wsproxy reporter. statsCollectorOptions := coderdAPI.Options.WorkspaceAppsStatsCollectorOptions statsCollectorOptions.Reporter = nil + if options.Flush != nil { + statsCollectorOptions.Flush = options.Flush + } wssrv, err := wsproxy.New(ctx, &wsproxy.Options{ Logger: slogtest.Make(t, nil).Leveled(slog.LevelDebug), diff --git a/enterprise/wsproxy/wsproxy_test.go b/enterprise/wsproxy/wsproxy_test.go index 312fdf98be047..9922258a8cff9 100644 --- a/enterprise/wsproxy/wsproxy_test.go +++ b/enterprise/wsproxy/wsproxy_test.go @@ -442,6 +442,13 @@ func TestWorkspaceProxyWorkspaceApps(t *testing.T) { "*", } + proxyFlushCh := make(chan chan<- struct{}, 1) + flushStats := func() { + proxyFlushDone := make(chan struct{}, 1) + proxyFlushCh <- proxyFlushDone + <-proxyFlushDone + } + client, closer, api, user := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ Options: &coderdtest.Options{ DeploymentValues: deploymentValues, @@ -476,6 +483,7 @@ func TestWorkspaceProxyWorkspaceApps(t *testing.T) { Name: "best-proxy", AppHostname: opts.AppHost, DisablePathApps: opts.DisablePathApps, + Flush: proxyFlushCh, }) return &apptest.Deployment{ @@ -483,6 +491,7 @@ func TestWorkspaceProxyWorkspaceApps(t *testing.T) { SDKClient: client, FirstUser: user, PathAppBaseURL: proxyAPI.Options.AccessURL, + FlushStats: flushStats, } }) } From 184af274bef2e0602d3b93b588219b65dc687273 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 15 Jan 2024 15:40:05 +0000 Subject: [PATCH 06/11] use unbuffered channels for flush --- coderd/workspaceapps/apptest/apptest.go | 4 ++-- coderd/workspaceapps_test.go | 2 +- enterprise/wsproxy/wsproxy_test.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index cc7958b66be35..5e8cfeac7fd77 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -147,7 +147,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.True(t, loc.Query().Has("message")) require.True(t, loc.Query().Has("redirect")) - assertWorkspaceLastUsedAtUpdated(t, appDetails) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("LoginWithoutAuthOnProxy", func(t *testing.T) { @@ -185,7 +185,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // request is getting stripped. require.Equal(t, u.Path, redirectURI.Path+"/") require.Equal(t, u.RawQuery, redirectURI.RawQuery) - assertWorkspaceLastUsedAtUpdated(t, appDetails) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("NoAccessShould404", func(t *testing.T) { diff --git a/coderd/workspaceapps_test.go b/coderd/workspaceapps_test.go index 3d7caf35c87de..ff9e530ce9b87 100644 --- a/coderd/workspaceapps_test.go +++ b/coderd/workspaceapps_test.go @@ -262,7 +262,7 @@ func TestWorkspaceApps(t *testing.T) { opts.AppHost = "" } - statsFlushCh := make(chan chan<- struct{}, 1) + statsFlushCh := make(chan chan<- struct{}) opts.StatsCollectorOptions.Flush = statsFlushCh flushStats := func() { flushDone := make(chan struct{}) diff --git a/enterprise/wsproxy/wsproxy_test.go b/enterprise/wsproxy/wsproxy_test.go index 9922258a8cff9..6cb260022f747 100644 --- a/enterprise/wsproxy/wsproxy_test.go +++ b/enterprise/wsproxy/wsproxy_test.go @@ -442,9 +442,9 @@ func TestWorkspaceProxyWorkspaceApps(t *testing.T) { "*", } - proxyFlushCh := make(chan chan<- struct{}, 1) + proxyFlushCh := make(chan chan<- struct{}) flushStats := func() { - proxyFlushDone := make(chan struct{}, 1) + proxyFlushDone := make(chan struct{}) proxyFlushCh <- proxyFlushDone <-proxyFlushDone } From 1f9012e4195f61327b8dc32854fd85a6c791fda4 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 15 Jan 2024 16:44:03 +0000 Subject: [PATCH 07/11] maybe fix race conditions? --- coderd/workspaceapps/apptest/apptest.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 5e8cfeac7fd77..8a15dc7d166fd 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -147,7 +147,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.True(t, loc.Query().Has("message")) require.True(t, loc.Query().Has("redirect")) - assertWorkspaceLastUsedAtNotUpdated(t, appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("LoginWithoutAuthOnProxy", func(t *testing.T) { @@ -185,7 +185,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // request is getting stripped. require.Equal(t, u.Path, redirectURI.Path+"/") require.Equal(t, u.RawQuery, redirectURI.RawQuery) - assertWorkspaceLastUsedAtNotUpdated(t, appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("NoAccessShould404", func(t *testing.T) { @@ -396,7 +396,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // TODO(@deansheather): This should be 400. There's a todo in the // resolve request code to fix this. require.Equal(t, http.StatusInternalServerError, resp.StatusCode) - assertWorkspaceLastUsedAtNotUpdated(t, appDetails) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) }) @@ -1570,9 +1570,13 @@ func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) { t.Helper() details.FlushStats() - ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) - require.NoError(t, err) - require.Greater(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt not updated when it should have been") + // Despite our efforts with the flush channel, this is inherently racy. + // Retry a few times to ensure that the stats collection pipeline has flushed. + require.Eventually(t, func() bool { + ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) + assert.NoError(t, err) + return ws.LastUsedAt.After(details.Workspace.LastUsedAt) + }, testutil.WaitShort, testutil.IntervalMedium, "workspace LastUsedAt not updated when it should have been") } // Except when it sometimes shouldn't (e.g. no access) @@ -1580,6 +1584,9 @@ func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, details *Details) { t.Helper() details.FlushStats() + // Despite our efforts with the flush channel, this is inherently racy. + // Wait for a short time to ensure that the stats collection pipeline has flushed. + <-time.After(testutil.IntervalSlow) ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) require.NoError(t, err) require.Equal(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt updated when it should not have been") From 862b1ff231cc7bf1bfd09c2f847b8abde7717e9f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 15 Jan 2024 17:14:45 +0000 Subject: [PATCH 08/11] hack: wait for connections to fully close --- coderd/workspaceapps/apptest/apptest.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 8a15dc7d166fd..a9a5032867534 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -1566,12 +1566,14 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli } // Accessing an app should update the workspace's LastUsedAt. +// Despite our efforts with the flush channel, this is inherently racy. +// Retry a few times to ensure that the stats collection pipeline has flushed. func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) { t.Helper() + <-time.After(testutil.IntervalMedium) details.FlushStats() - // Despite our efforts with the flush channel, this is inherently racy. - // Retry a few times to ensure that the stats collection pipeline has flushed. + <-time.After(testutil.IntervalMedium) require.Eventually(t, func() bool { ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) assert.NoError(t, err) @@ -1580,13 +1582,14 @@ func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) { } // Except when it sometimes shouldn't (e.g. no access) +// Despite our efforts with the flush channel, this is inherently racy. +// Wait for a short time to ensure that the stats collection pipeline has flushed. func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, details *Details) { t.Helper() + <-time.After(testutil.IntervalMedium) // wait for connections to close details.FlushStats() - // Despite our efforts with the flush channel, this is inherently racy. - // Wait for a short time to ensure that the stats collection pipeline has flushed. - <-time.After(testutil.IntervalSlow) + <-time.After(testutil.IntervalMedium) ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) require.NoError(t, err) require.Equal(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt updated when it should not have been") From f522ea6fd6c5c32ef3e7ac5272a726b748866181 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 15 Jan 2024 17:16:09 +0000 Subject: [PATCH 09/11] reword comments --- coderd/workspaceapps/apptest/apptest.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index a9a5032867534..5fe8393ebec97 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -1566,14 +1566,13 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli } // Accessing an app should update the workspace's LastUsedAt. -// Despite our efforts with the flush channel, this is inherently racy. -// Retry a few times to ensure that the stats collection pipeline has flushed. +// NOTE: Despite our efforts with the flush channel, this is inherently racy. func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) { t.Helper() - <-time.After(testutil.IntervalMedium) + <-time.After(testutil.IntervalMedium) // Wait for Bicopy to finish. details.FlushStats() - <-time.After(testutil.IntervalMedium) + // Wait for stats to fully flush. require.Eventually(t, func() bool { ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) assert.NoError(t, err) @@ -1582,14 +1581,13 @@ func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) { } // Except when it sometimes shouldn't (e.g. no access) -// Despite our efforts with the flush channel, this is inherently racy. -// Wait for a short time to ensure that the stats collection pipeline has flushed. +// NOTE: Despite our efforts with the flush channel, this is inherently racy. func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, details *Details) { t.Helper() - <-time.After(testutil.IntervalMedium) // wait for connections to close + <-time.After(testutil.IntervalMedium) // Wait for Bicopy to finish. details.FlushStats() - <-time.After(testutil.IntervalMedium) + <-time.After(testutil.IntervalMedium) // Wait for stats to fully flush. ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) require.NoError(t, err) require.Equal(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt updated when it should not have been") From b714d69b155e651aa87a846342c0eeee94817a34 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 16 Jan 2024 09:47:47 +0000 Subject: [PATCH 10/11] improve comment --- coderd/workspaceapps/stats.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/coderd/workspaceapps/stats.go b/coderd/workspaceapps/stats.go index 9bf469e3b2b2a..76a60c6fbb5df 100644 --- a/coderd/workspaceapps/stats.go +++ b/coderd/workspaceapps/stats.go @@ -126,7 +126,11 @@ func (r *StatsDBReporter) Report(ctx context.Context, stats []StatsReport) error return err } - // TODO: There should be a better way to do this. + // TODO: We currently measure workspace usage based on when we get stats from it. + // There are currently two paths for this: + // 1) From SSH -> workspace agent stats POSTed from agent + // 2) From workspace apps / rpty -> workspace app stats (from coderd / wsproxy) + // Ideally we would have a single code path for this. uniqueIDs := slice.Unique(batch.WorkspaceID) if err := tx.BatchUpdateWorkspaceLastUsedAt(ctx, database.BatchUpdateWorkspaceLastUsedAtParams{ IDs: uniqueIDs, From 8573ba729efa77d7aef992bce797919dbb6ada62 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 16 Jan 2024 13:33:55 +0000 Subject: [PATCH 11/11] address PR comments --- coderd/database/dbmem/dbmem.go | 3 --- coderd/workspaceapps/apptest/apptest.go | 5 +---- coderd/workspaceapps_test.go | 10 +++++----- enterprise/coderd/coderdenttest/proxytest.go | 8 ++++---- enterprise/wsproxy/wsproxy_test.go | 10 +++++----- 5 files changed, 15 insertions(+), 21 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 9178f561255df..f378ae9610ffc 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -985,9 +985,6 @@ func (q *FakeQuerier) BatchUpdateWorkspaceLastUsedAt(_ context.Context, arg data q.workspaces[i].LastUsedAt = arg.LastUsedAt n++ } - if n == 0 { - return sql.ErrNoRows - } return nil } diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 5fe8393ebec97..a8c593c1ec89d 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -1570,10 +1570,9 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) { t.Helper() - <-time.After(testutil.IntervalMedium) // Wait for Bicopy to finish. - details.FlushStats() // Wait for stats to fully flush. require.Eventually(t, func() bool { + details.FlushStats() ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) assert.NoError(t, err) return ws.LastUsedAt.After(details.Workspace.LastUsedAt) @@ -1585,9 +1584,7 @@ func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) { func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, details *Details) { t.Helper() - <-time.After(testutil.IntervalMedium) // Wait for Bicopy to finish. details.FlushStats() - <-time.After(testutil.IntervalMedium) // Wait for stats to fully flush. ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) require.NoError(t, err) require.Equal(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt updated when it should not have been") diff --git a/coderd/workspaceapps_test.go b/coderd/workspaceapps_test.go index ff9e530ce9b87..341cc3bc56031 100644 --- a/coderd/workspaceapps_test.go +++ b/coderd/workspaceapps_test.go @@ -262,12 +262,12 @@ func TestWorkspaceApps(t *testing.T) { opts.AppHost = "" } - statsFlushCh := make(chan chan<- struct{}) - opts.StatsCollectorOptions.Flush = statsFlushCh + flushStatsCollectorCh := make(chan chan<- struct{}, 1) + opts.StatsCollectorOptions.Flush = flushStatsCollectorCh flushStats := func() { - flushDone := make(chan struct{}) - statsFlushCh <- flushDone - <-flushDone + flushStatsCollectorDone := make(chan struct{}, 1) + flushStatsCollectorCh <- flushStatsCollectorDone + <-flushStatsCollectorDone } client := coderdtest.New(t, &coderdtest.Options{ DeploymentValues: deploymentValues, diff --git a/enterprise/coderd/coderdenttest/proxytest.go b/enterprise/coderd/coderdenttest/proxytest.go index 36dc287798ae1..e93e051a20b59 100644 --- a/enterprise/coderd/coderdenttest/proxytest.go +++ b/enterprise/coderd/coderdenttest/proxytest.go @@ -38,8 +38,8 @@ type ProxyOptions struct { // ProxyURL is optional ProxyURL *url.URL - // Flush is optional - Flush chan chan<- struct{} + // FlushStats is optional + FlushStats chan chan<- struct{} } // NewWorkspaceProxy will configure a wsproxy.Server with the given options. @@ -116,8 +116,8 @@ func NewWorkspaceProxy(t *testing.T, coderdAPI *coderd.API, owner *codersdk.Clie // Inherit collector options from coderd, but keep the wsproxy reporter. statsCollectorOptions := coderdAPI.Options.WorkspaceAppsStatsCollectorOptions statsCollectorOptions.Reporter = nil - if options.Flush != nil { - statsCollectorOptions.Flush = options.Flush + if options.FlushStats != nil { + statsCollectorOptions.Flush = options.FlushStats } wssrv, err := wsproxy.New(ctx, &wsproxy.Options{ diff --git a/enterprise/wsproxy/wsproxy_test.go b/enterprise/wsproxy/wsproxy_test.go index 6cb260022f747..46f00d9752587 100644 --- a/enterprise/wsproxy/wsproxy_test.go +++ b/enterprise/wsproxy/wsproxy_test.go @@ -442,11 +442,11 @@ func TestWorkspaceProxyWorkspaceApps(t *testing.T) { "*", } - proxyFlushCh := make(chan chan<- struct{}) + proxyStatsCollectorFlushCh := make(chan chan<- struct{}, 1) flushStats := func() { - proxyFlushDone := make(chan struct{}) - proxyFlushCh <- proxyFlushDone - <-proxyFlushDone + proxyStatsCollectorFlushDone := make(chan struct{}, 1) + proxyStatsCollectorFlushCh <- proxyStatsCollectorFlushDone + <-proxyStatsCollectorFlushDone } client, closer, api, user := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ @@ -483,7 +483,7 @@ func TestWorkspaceProxyWorkspaceApps(t *testing.T) { Name: "best-proxy", AppHostname: opts.AppHost, DisablePathApps: opts.DisablePathApps, - Flush: proxyFlushCh, + FlushStats: proxyStatsCollectorFlushCh, }) return &apptest.Deployment{