Skip to content

Commit 40cd260

Browse files
ammarioammario
committed
Make concurrency more robust
1 parent 2e82543 commit 40cd260

File tree

5 files changed

+22
-15
lines changed

5 files changed

+22
-15
lines changed

‎agent/agent.go

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"go.uber.org/atomic"
3636
gossh "golang.org/x/crypto/ssh"
3737
"golang.org/x/exp/slices"
38+
"golang.org/x/sync/singleflight"
3839
"golang.org/x/xerrors"
3940
"tailscale.com/net/speedtest"
4041
"tailscale.com/tailcfg"
@@ -285,6 +286,8 @@ func (a *agent) reportMetadataLoop(ctx context.Context) {
285286
)
286287
defer baseTicker.Stop()
287288

289+
var flight singleflight.Group
290+
288291
for {
289292
select {
290293
case <-ctx.Done():
@@ -351,20 +354,25 @@ func (a *agent) reportMetadataLoop(ctx context.Context) {
351354
}
352355
}
353356

354-
go func(md codersdk.WorkspaceAgentMetadataDescription) {
357+
md := md
358+
// We send the result to the channel in the goroutine to avoid
359+
// sending the same result multiple times. So, we don't care about
360+
// the return values.
361+
flight.DoChan(md.Key, func() (interface{}, error) {
355362
select {
356363
case <-ctx.Done():
357-
return
364+
return 0, nil
358365
case metadataResults <- metadataResultAndKey{
359366
key: md.Key,
360367
result: a.collectMetadata(ctx, md),
361368
}:
362369
default:
363-
// This should be impossible because the channel is empty
364-
// before we start spinning up send goroutines.
370+
// This should be impossible because the channel is confirmed
371+
// to be empty before this goroutine is spawned.
365372
a.logger.Error(ctx, "metadataResults channel full")
366373
}
367-
}(md)
374+
return 0, nil
375+
})
368376
}
369377
}
370378
}

‎agent/agent_test.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -960,9 +960,8 @@ func TestAgent_Metadata(t *testing.T) {
960960

961961
dir := t.TempDir()
962962

963-
// In tests, the interval unit is 100 milliseconds while in production
964-
// it is 1 second.
965963
const reportInterval = 2
964+
const intervalUnit = 100 * time.Millisecond
966965
var (
967966
greetingPath = filepath.Join(dir, "greeting")
968967
script = "echo hello | tee -a " + greetingPath
@@ -986,7 +985,7 @@ func TestAgent_Metadata(t *testing.T) {
986985
return len(client.getMetadata()) == 2
987986
}, testutil.WaitShort, testutil.IntervalMedium)
988987

989-
for start := time.Now(); time.Since(start) < testutil.WaitShort; time.Sleep(testutil.IntervalMedium) {
988+
for start := time.Now(); time.Since(start) < testutil.WaitMedium; time.Sleep(testutil.IntervalMedium) {
990989
md := client.getMetadata()
991990
if len(md) != 2 {
992991
panic("unexpected number of metadata entries")
@@ -1000,7 +999,7 @@ func TestAgent_Metadata(t *testing.T) {
1000999

10011000
var (
10021001
numGreetings = bytes.Count(greetingByt, []byte("hello"))
1003-
idealNumGreetings = time.Since(start) / (reportInterval * 100 * time.Millisecond)
1002+
idealNumGreetings = time.Since(start) / (reportInterval * intervalUnit)
10041003
// We allow a 50% error margin because the report loop may backlog
10051004
// in CI and other toasters. In production, there is no hard
10061005
// guarantee on timing either, and the frontend gives similar
@@ -1009,7 +1008,7 @@ func TestAgent_Metadata(t *testing.T) {
10091008
lowerBound = (int(idealNumGreetings) / 2)
10101009
)
10111010

1012-
if idealNumGreetings < 5 {
1011+
if idealNumGreetings < 50 {
10131012
// There is an insufficient sample size.
10141013
continue
10151014
}
@@ -1018,7 +1017,7 @@ func TestAgent_Metadata(t *testing.T) {
10181017
// The report loop may slow down on load, but it should never, ever
10191018
// speed up.
10201019
if numGreetings > upperBound {
1021-
t.Fatalf("too many greetings: %d > %d", numGreetings, upperBound)
1020+
t.Fatalf("too many greetings: %d > %d in %v", numGreetings, upperBound, time.Since(start))
10221021
} else if numGreetings < lowerBound {
10231022
t.Fatalf("too few greetings: %d < %d", numGreetings, lowerBound)
10241023
}

‎coderd/database/dbfake/databasefake.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2762,7 +2762,7 @@ func (q *fakeQuerier) UpdateWorkspaceAgentMetadata(_ context.Context, arg databa
27622762
}
27632763
}
27642764

2765-
return sql.ErrNoRows
2765+
return nil
27662766
}
27672767

27682768
func (q *fakeQuerier) InsertWorkspaceAgentMetadata(_ context.Context, arg database.InsertWorkspaceAgentMetadataParams) error {

‎coderd/workspaceagents.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1356,7 +1356,7 @@ func (api *API) workspaceAgentPostMetadata(rw http.ResponseWriter, r *http.Reque
13561356
key := chi.URLParam(r, "key")
13571357

13581358
const (
1359-
maxValueLen = 64 << 10
1359+
maxValueLen = 32 << 10
13601360
maxErrorLen = maxValueLen
13611361
)
13621362

‎coderd/workspaceagents_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1400,7 +1400,7 @@ func TestWorkspaceAgent_Metadata(t *testing.T) {
14001400
require.Len(t, update, 3)
14011401
check(wantMetadata1, update[0])
14021402

1403-
const maxValueLen = 64 << 10
1403+
const maxValueLen = 32 << 10
14041404
tooLongValueMetadata := wantMetadata1
14051405
tooLongValueMetadata.Value = strings.Repeat("a", maxValueLen*2)
14061406
tooLongValueMetadata.Error = ""
@@ -1412,5 +1412,5 @@ func TestWorkspaceAgent_Metadata(t *testing.T) {
14121412

14131413
unknownKeyMetadata := wantMetadata1
14141414
err = agentClient.PostMetadata(ctx, "unknown", unknownKeyMetadata)
1415-
require.Error(t, err)
1415+
require.NoError(t, err)
14161416
}

0 commit comments

Comments
 (0)