Skip to content

Commit 2f71aaa

Browse files
ethanndicksonethanndickson
committed
review
1 parent 7246650 commit 2f71aaa

File tree

2 files changed

+77
-57
lines changed

2 files changed

+77
-57
lines changed

‎coderd/metricscache/metricscache.go

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package metricscache
33
import (
44
"context"
55
"database/sql"
6-
"slices"
76
"sync"
87
"sync/atomic"
98
"time"
@@ -35,9 +34,8 @@ type Cache struct {
3534
templateAverageBuildTime atomic.Pointer[map[uuid.UUID]database.GetTemplateAverageBuildTimeRow]
3635
deploymentStatsResponse atomic.Pointer[codersdk.DeploymentStats]
3736

38-
cancel func()
39-
tickersMu sync.Mutex
40-
tickers []quartz.Waiter
37+
done chan struct{}
38+
cancel func()
4139

4240
// usage is a experiment flag to enable new workspace usage tracking behavior and will be
4341
// removed when the experiment is complete.
@@ -63,13 +61,25 @@ func New(db database.Store, log slog.Logger, clock quartz.Clock, intervals Inter
6361
database: db,
6462
intervals: intervals,
6563
log: log,
64+
done: make(chan struct{}),
6665
cancel: cancel,
6766
usage: usage,
6867
}
69-
70-
go c.run(ctx, "template build times", intervals.TemplateBuildTimes, c.refreshTemplateBuildTimes)
71-
go c.run(ctx, "deployment stats", intervals.DeploymentStats, c.refreshDeploymentStats)
72-
68+
go func() {
69+
var wg sync.WaitGroup
70+
defer close(c.done)
71+
wg.Add(1)
72+
go func() {
73+
defer wg.Done()
74+
c.run(ctx, "template build times", intervals.TemplateBuildTimes, c.refreshTemplateBuildTimes)
75+
}()
76+
wg.Add(1)
77+
go func() {
78+
defer wg.Done()
79+
c.run(ctx, "deployment stats", intervals.DeploymentStats, c.refreshDeploymentStats)
80+
}()
81+
wg.Wait()
82+
}()
7383
return c
7484
}
7585

@@ -196,20 +206,12 @@ func (c *Cache) run(ctx context.Context, name string, interval time.Duration, re
196206
_ = tickerFunc()
197207

198208
tkr := c.clock.TickerFunc(ctx, interval, tickerFunc, "metricscache", name)
199-
c.tickersMu.Lock()
200-
c.tickers = append(c.tickers, tkr)
201-
c.tickersMu.Unlock()
209+
_ = tkr.Wait()
202210
}
203211

204212
func (c *Cache) Close() error {
205213
c.cancel()
206-
c.tickersMu.Lock()
207-
tickers := slices.Clone(c.tickers)
208-
c.tickersMu.Unlock()
209-
210-
for _, tkr := range tickers {
211-
_ = tkr.Wait()
212-
}
214+
<-c.done
213215
return nil
214216
}
215217

‎coderd/metricscache/metricscache_test.go

Lines changed: 57 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,21 @@ func newMetricsCache(t *testing.T, log slog.Logger, clock quartz.Clock, interval
4949

5050
func TestCache_TemplateWorkspaceOwners(t *testing.T) {
5151
t.Parallel()
52-
var ()
5352

5453
var (
55-
log = testutil.Logger(t)
56-
clock = quartz.NewReal()
57-
cache, db = newMetricsCache(t, log, clock, metricscache.Intervals{
58-
TemplateBuildTimes: testutil.IntervalFast,
59-
}, false)
54+
ctx = testutil.Context(t, testutil.WaitShort)
55+
log = testutil.Logger(t)
56+
clock = quartz.NewMock(t)
6057
)
6158

59+
trapTickerFunc := clock.Trap().TickerFunc("metricscache")
60+
defer trapTickerFunc.Close()
61+
62+
cache, db := newMetricsCache(t, log, clock, metricscache.Intervals{
63+
TemplateBuildTimes: time.Minute,
64+
DeploymentStats: time.Minute,
65+
}, false)
66+
6267
org := dbgen.Organization(t, db, database.Organization{})
6368
user1 := dbgen.User(t, db, database.User{})
6469
user2 := dbgen.User(t, db, database.User{})
@@ -67,38 +72,47 @@ func TestCache_TemplateWorkspaceOwners(t *testing.T) {
6772
Provisioner: database.ProvisionerTypeEcho,
6873
CreatedBy: user1.ID,
6974
})
70-
require.Eventuallyf(t, func() bool {
71-
count, ok := cache.TemplateWorkspaceOwners(template.ID)
72-
return ok && count == 0
73-
}, testutil.WaitShort, testutil.IntervalMedium,
74-
"TemplateWorkspaceOwners never populated 0 owners",
75-
)
75+
76+
// Wait for both ticker functions to be created (template build times and deployment stats)
77+
trapTickerFunc.MustWait(ctx).MustRelease(ctx)
78+
trapTickerFunc.MustWait(ctx).MustRelease(ctx)
79+
80+
_, wait := clock.AdvanceNext()
81+
wait.MustWait(ctx)
82+
_, wait = clock.AdvanceNext()
83+
wait.MustWait(ctx)
84+
85+
count, ok := cache.TemplateWorkspaceOwners(template.ID)
86+
require.True(t, ok, "TemplateWorkspaceOwners should be populated")
87+
require.Equal(t, 0, count, "should have 0 owners initially")
7688

7789
dbgen.Workspace(t, db, database.WorkspaceTable{
7890
OrganizationID: org.ID,
7991
TemplateID: template.ID,
8092
OwnerID: user1.ID,
8193
})
8294

83-
require.Eventuallyf(t, func() bool {
84-
count, _ := cache.TemplateWorkspaceOwners(template.ID)
85-
return count == 1
86-
}, testutil.WaitShort, testutil.IntervalMedium,
87-
"TemplateWorkspaceOwners never populated 1 owner",
88-
)
95+
_, wait = clock.AdvanceNext()
96+
wait.MustWait(ctx)
97+
_, wait = clock.AdvanceNext()
98+
wait.MustWait(ctx)
99+
100+
count, _ = cache.TemplateWorkspaceOwners(template.ID)
101+
require.Equal(t, 1, count, "should have 1 owner after adding workspace")
89102

90103
workspace2 := dbgen.Workspace(t, db, database.WorkspaceTable{
91104
OrganizationID: org.ID,
92105
TemplateID: template.ID,
93106
OwnerID: user2.ID,
94107
})
95108

96-
require.Eventuallyf(t, func() bool {
97-
count, _ := cache.TemplateWorkspaceOwners(template.ID)
98-
return count == 2
99-
}, testutil.WaitShort, testutil.IntervalMedium,
100-
"TemplateWorkspaceOwners never populated 2 owners",
101-
)
109+
_, wait = clock.AdvanceNext()
110+
wait.MustWait(ctx)
111+
_, wait = clock.AdvanceNext()
112+
wait.MustWait(ctx)
113+
114+
count, _ = cache.TemplateWorkspaceOwners(template.ID)
115+
require.Equal(t, 2, count, "should have 2 owners after adding second workspace")
102116

103117
// 3rd workspace should not be counted since we have the same owner as workspace2.
104118
dbgen.Workspace(t, db, database.WorkspaceTable{
@@ -112,12 +126,13 @@ func TestCache_TemplateWorkspaceOwners(t *testing.T) {
112126
Deleted: true,
113127
})
114128

115-
require.Eventuallyf(t, func() bool {
116-
count, _ := cache.TemplateWorkspaceOwners(template.ID)
117-
return count == 1
118-
}, testutil.WaitShort, testutil.IntervalMedium,
119-
"TemplateWorkspaceOwners never populated 1 owner after delete",
120-
)
129+
_, wait = clock.AdvanceNext()
130+
wait.MustWait(ctx)
131+
_, wait = clock.AdvanceNext()
132+
wait.MustWait(ctx)
133+
134+
count, _ = cache.TemplateWorkspaceOwners(template.ID)
135+
require.Equal(t, 1, count, "should have 1 owner after deleting workspace")
121136
}
122137

123138
func clockTime(t time.Time, hour, minute, sec int) time.Time {
@@ -205,22 +220,22 @@ func TestCache_BuildTime(t *testing.T) {
205220
t.Run(tt.name, func(t *testing.T) {
206221
t.Parallel()
207222

208-
ctx := testutil.Context(t, testutil.WaitShort)
209-
210223
var (
224+
ctx = testutil.Context(t, testutil.WaitShort)
211225
log = testutil.Logger(t)
212226
clock = quartz.NewMock(t)
213227
)
214228

229+
clock.Set(someDay)
230+
215231
trapTickerFunc := clock.Trap().TickerFunc("metricscache")
216232

217233
defer trapTickerFunc.Close()
218234
cache, db := newMetricsCache(t, log, clock, metricscache.Intervals{
219-
TemplateBuildTimes: testutil.IntervalFast,
235+
TemplateBuildTimes: time.Minute,
236+
DeploymentStats: time.Minute,
220237
}, false)
221238

222-
clock.Set(someDay)
223-
224239
org := dbgen.Organization(t, db, database.Organization{})
225240
user := dbgen.User(t, db, database.User{})
226241

@@ -269,6 +284,8 @@ func TestCache_BuildTime(t *testing.T) {
269284

270285
_, wait := clock.AdvanceNext()
271286
wait.MustWait(ctx)
287+
_, wait = clock.AdvanceNext()
288+
wait.MustWait(ctx)
272289

273290
if tt.want.loads {
274291
wantTransition := codersdk.WorkspaceTransition(tt.args.transition)
@@ -295,9 +312,8 @@ func TestCache_BuildTime(t *testing.T) {
295312
func TestCache_DeploymentStats(t *testing.T) {
296313
t.Parallel()
297314

298-
ctx := testutil.Context(t, testutil.WaitShort)
299-
300315
var (
316+
ctx = testutil.Context(t, testutil.WaitShort)
301317
log = testutil.Logger(t)
302318
clock = quartz.NewMock(t)
303319
)
@@ -306,8 +322,8 @@ func TestCache_DeploymentStats(t *testing.T) {
306322
defer tickerTrap.Close()
307323

308324
cache, db := newMetricsCache(t, log, clock, metricscache.Intervals{
309-
TemplateBuildTimes: testutil.IntervalFast,
310-
DeploymentStats: testutil.IntervalFast,
325+
TemplateBuildTimes: time.Minute,
326+
DeploymentStats: time.Minute,
311327
}, false)
312328

313329
err := db.InsertWorkspaceAgentStats(context.Background(), database.InsertWorkspaceAgentStatsParams{
@@ -339,6 +355,8 @@ func TestCache_DeploymentStats(t *testing.T) {
339355

340356
_, wait := clock.AdvanceNext()
341357
wait.MustWait(ctx)
358+
_, wait = clock.AdvanceNext()
359+
wait.MustWait(ctx)
342360

343361
stat, ok := cache.DeploymentStats()
344362
require.True(t, ok, "cache should be populated after refresh")

0 commit comments

Comments
 (0)