test: fix TestCache_DeploymentStats flake (#19683)

Closes https://github.com/coder/internal/issues/961
Likely the same deal as in #19599, the body of `require.Eventually` now fires immediately, when it used to fire after 250ms (the interval). Presumably, the deployment stats become ready before the vs code session count gets incremented. This was never an issue with the 250ms delay, as this flake has only cropped up after the testify version bump.

We'll fix the issue by making it possible to wait for a full metrics cache refresh, i.e. removing `require.Eventually` in this test altogether.

coderd/coderd.go

@@ -459,7 +459,7 @@ func New(options *Options) *API {
 	metricsCache := metricscache.New(
 		options.Database,
 		options.Logger.Named("metrics_cache"),
-		options.Clock,
+		quartz.NewReal(),
 		metricscache.Intervals{
 			TemplateBuildTimes: options.MetricsCacheRefreshInterval,
 			DeploymentStats:    options.AgentStatsRefreshInterval,

coderd/metricscache/metricscache.go

@@ -181,16 +181,14 @@ func (c *Cache) refreshDeploymentStats(ctx context.Context) error {
 
 func (c *Cache) run(ctx context.Context, name string, interval time.Duration, refresh func(context.Context) error) {
 	logger := c.log.With(slog.F("name", name), slog.F("interval", interval))
-	ticker := time.NewTicker(interval)
-	defer ticker.Stop()
 
-	for {
+	tickerFunc := func() error {
+		start := c.clock.Now()
 		for r := retry.New(time.Millisecond*100, time.Minute); r.Wait(ctx); {
-			start := time.Now()
 			err := refresh(ctx)
 			if err != nil {
 				if ctx.Err() != nil {
-					return
+					return nil
 				}
 				if xerrors.Is(err, sql.ErrNoRows) {
 					break
@@ -198,18 +196,17 @@ func (c *Cache) run(ctx context.Context, name string, interval time.Duration, re
 				logger.Error(ctx, "refresh metrics failed", slog.Error(err))
 				continue
 			}
-			logger.Debug(ctx, "metrics refreshed", slog.F("took", time.Since(start)))
+			logger.Debug(ctx, "metrics refreshed", slog.F("took", c.clock.Since(start)))
 			break
 		}
-
-		select {
-		case <-ticker.C:
-		case <-c.done:
-			return
-		case <-ctx.Done():
-			return
-		}
+		return nil
 	}
+
+	// Call once immediately before starting ticker
+	_ = tickerFunc()
+
+	tkr := c.clock.TickerFunc(ctx, interval, tickerFunc, "metricscache", name)
+	_ = tkr.Wait()
 }
 
 func (c *Cache) Close() error {

coderd/metricscache/metricscache_test.go

@@ -49,16 +49,20 @@ func newMetricsCache(t *testing.T, log slog.Logger, clock quartz.Clock, interval
 
 func TestCache_TemplateWorkspaceOwners(t *testing.T) {
 	t.Parallel()
-	var ()
 
 	var (
-		log       = testutil.Logger(t)
-		clock     = quartz.NewReal()
-		cache, db = newMetricsCache(t, log, clock, metricscache.Intervals{
-			TemplateBuildTimes: testutil.IntervalFast,
-		}, false)
+		ctx   = testutil.Context(t, testutil.WaitShort)
+		log   = testutil.Logger(t)
+		clock = quartz.NewMock(t)
 	)
 
+	trapTickerFunc := clock.Trap().TickerFunc("metricscache")
+	defer trapTickerFunc.Close()
+
+	cache, db := newMetricsCache(t, log, clock, metricscache.Intervals{
+		TemplateBuildTimes: time.Minute,
+	}, false)
+
 	org := dbgen.Organization(t, db, database.Organization{})
 	user1 := dbgen.User(t, db, database.User{})
 	user2 := dbgen.User(t, db, database.User{})
@@ -67,12 +71,16 @@ func TestCache_TemplateWorkspaceOwners(t *testing.T) {
 		Provisioner:    database.ProvisionerTypeEcho,
 		CreatedBy:      user1.ID,
 	})
-	require.Eventuallyf(t, func() bool {
-		count, ok := cache.TemplateWorkspaceOwners(template.ID)
-		return ok && count == 0
-	}, testutil.WaitShort, testutil.IntervalMedium,
-		"TemplateWorkspaceOwners never populated 0 owners",
-	)
+
+	// Wait for both ticker functions to be created (template build times and deployment stats)
+	trapTickerFunc.MustWait(ctx).MustRelease(ctx)
+	trapTickerFunc.MustWait(ctx).MustRelease(ctx)
+
+	clock.Advance(time.Minute).MustWait(ctx)
+
+	count, ok := cache.TemplateWorkspaceOwners(template.ID)
+	require.True(t, ok, "TemplateWorkspaceOwners should be populated")
+	require.Equal(t, 0, count, "should have 0 owners initially")
 
 	dbgen.Workspace(t, db, database.WorkspaceTable{
 		OrganizationID: org.ID,
@@ -80,12 +88,10 @@ func TestCache_TemplateWorkspaceOwners(t *testing.T) {
 		OwnerID:        user1.ID,
 	})
 
-	require.Eventuallyf(t, func() bool {
-		count, _ := cache.TemplateWorkspaceOwners(template.ID)
-		return count == 1
-	}, testutil.WaitShort, testutil.IntervalMedium,
-		"TemplateWorkspaceOwners never populated 1 owner",
-	)
+	clock.Advance(time.Minute).MustWait(ctx)
+
+	count, _ = cache.TemplateWorkspaceOwners(template.ID)
+	require.Equal(t, 1, count, "should have 1 owner after adding workspace")
 
 	workspace2 := dbgen.Workspace(t, db, database.WorkspaceTable{
 		OrganizationID: org.ID,
@@ -93,12 +99,10 @@ func TestCache_TemplateWorkspaceOwners(t *testing.T) {
 		OwnerID:        user2.ID,
 	})
 
-	require.Eventuallyf(t, func() bool {
-		count, _ := cache.TemplateWorkspaceOwners(template.ID)
-		return count == 2
-	}, testutil.WaitShort, testutil.IntervalMedium,
-		"TemplateWorkspaceOwners never populated 2 owners",
-	)
+	clock.Advance(time.Minute).MustWait(ctx)
+
+	count, _ = cache.TemplateWorkspaceOwners(template.ID)
+	require.Equal(t, 2, count, "should have 2 owners after adding second workspace")
 
 	// 3rd workspace should not be counted since we have the same owner as workspace2.
 	dbgen.Workspace(t, db, database.WorkspaceTable{
@@ -112,12 +116,10 @@ func TestCache_TemplateWorkspaceOwners(t *testing.T) {
 		Deleted: true,
 	})
 
-	require.Eventuallyf(t, func() bool {
-		count, _ := cache.TemplateWorkspaceOwners(template.ID)
-		return count == 1
-	}, testutil.WaitShort, testutil.IntervalMedium,
-		"TemplateWorkspaceOwners never populated 1 owner after delete",
-	)
+	clock.Advance(time.Minute).MustWait(ctx)
+
+	count, _ = cache.TemplateWorkspaceOwners(template.ID)
+	require.Equal(t, 1, count, "should have 1 owner after deleting workspace")
 }
 
 func clockTime(t time.Time, hour, minute, sec int) time.Time {
@@ -206,15 +208,20 @@ func TestCache_BuildTime(t *testing.T) {
 			t.Parallel()
 
 			var (
-				log       = testutil.Logger(t)
-				clock     = quartz.NewMock(t)
-				cache, db = newMetricsCache(t, log, clock, metricscache.Intervals{
-					TemplateBuildTimes: testutil.IntervalFast,
-				}, false)
+				ctx   = testutil.Context(t, testutil.WaitShort)
+				log   = testutil.Logger(t)
+				clock = quartz.NewMock(t)
 			)
 
 			clock.Set(someDay)
 
+			trapTickerFunc := clock.Trap().TickerFunc("metricscache")
+
+			defer trapTickerFunc.Close()
+			cache, db := newMetricsCache(t, log, clock, metricscache.Intervals{
+				TemplateBuildTimes: time.Minute,
+			}, false)
+
 			org := dbgen.Organization(t, db, database.Organization{})
 			user := dbgen.User(t, db, database.User{})
 
@@ -257,17 +264,19 @@ func TestCache_BuildTime(t *testing.T) {
 				})
 			}
 
+			// Wait for both ticker functions to be created (template build times and deployment stats)
+			trapTickerFunc.MustWait(ctx).MustRelease(ctx)
+			trapTickerFunc.MustWait(ctx).MustRelease(ctx)
+
+			clock.Advance(time.Minute).MustWait(ctx)
+
 			if tt.want.loads {
 				wantTransition := codersdk.WorkspaceTransition(tt.args.transition)
-				require.Eventuallyf(t, func() bool {
-					stats := cache.TemplateBuildTimeStats(template.ID)
-					ts := stats[wantTransition]
-					return ts.P50 != nil && *ts.P50 == tt.want.buildTimeMs
-				}, testutil.WaitLong, testutil.IntervalMedium,
-					"P50 never reached expected value for %v", wantTransition,
-				)
-
 				gotStats := cache.TemplateBuildTimeStats(template.ID)
+				ts := gotStats[wantTransition]
+				require.NotNil(t, ts.P50, "P50 should be set for %v", wantTransition)
+				require.Equal(t, tt.want.buildTimeMs, *ts.P50, "P50 should match expected value for %v", wantTransition)
+
 				for transition, ts := range gotStats {
 					if transition == wantTransition {
 						// Checked above
@@ -276,14 +285,8 @@ func TestCache_BuildTime(t *testing.T) {
 					require.Empty(t, ts, "%v", transition)
 				}
 			} else {
-				var stats codersdk.TemplateBuildTimeStats
-				require.Never(t, func() bool {
-					stats = cache.TemplateBuildTimeStats(template.ID)
-					requireBuildTimeStatsEmpty(t, stats)
-					return t.Failed()
-				}, testutil.WaitShort/2, testutil.IntervalMedium,
-					"BuildTimeStats populated", stats,
-				)
+				stats := cache.TemplateBuildTimeStats(template.ID)
+				requireBuildTimeStatsEmpty(t, stats)
 			}
 		})
 	}
@@ -293,13 +296,18 @@ func TestCache_DeploymentStats(t *testing.T) {
 	t.Parallel()
 
 	var (
-		log       = testutil.Logger(t)
-		clock     = quartz.NewMock(t)
-		cache, db = newMetricsCache(t, log, clock, metricscache.Intervals{
-			DeploymentStats: testutil.IntervalFast,
-		}, false)
+		ctx   = testutil.Context(t, testutil.WaitShort)
+		log   = testutil.Logger(t)
+		clock = quartz.NewMock(t)
 	)
 
+	tickerTrap := clock.Trap().TickerFunc("metricscache")
+	defer tickerTrap.Close()
+
+	cache, db := newMetricsCache(t, log, clock, metricscache.Intervals{
+		DeploymentStats: time.Minute,
+	}, false)
+
 	err := db.InsertWorkspaceAgentStats(context.Background(), database.InsertWorkspaceAgentStatsParams{
 		ID:                 []uuid.UUID{uuid.New()},
 		CreatedAt:          []time.Time{clock.Now()},
@@ -323,11 +331,13 @@ func TestCache_DeploymentStats(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	var stat codersdk.DeploymentStats
-	require.Eventually(t, func() bool {
-		var ok bool
-		stat, ok = cache.DeploymentStats()
-		return ok
-	}, testutil.WaitLong, testutil.IntervalMedium)
+	// Wait for both ticker functions to be created (template build times and deployment stats)
+	tickerTrap.MustWait(ctx).MustRelease(ctx)
+	tickerTrap.MustWait(ctx).MustRelease(ctx)
+
+	clock.Advance(time.Minute).MustWait(ctx)
+
+	stat, ok := cache.DeploymentStats()
+	require.True(t, ok, "cache should be populated after refresh")
 	require.Equal(t, int64(1), stat.SessionCount.VSCode)
 }