From 62b5efea1e728cb02d36bbd075ca01862527ffcb Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 31 Oct 2024 15:23:06 +0200 Subject: [PATCH 1/3] test(coderd/notifications): fix data races in tests and smpttest server --- coderd/notifications/dispatch/smtptest/server.go | 15 ++++++++++++++- coderd/notifications/notifications_test.go | 15 +++++++-------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/coderd/notifications/dispatch/smtptest/server.go b/coderd/notifications/dispatch/smtptest/server.go index 689b4d384036d..1aac9ecd55cfb 100644 --- a/coderd/notifications/dispatch/smtptest/server.go +++ b/coderd/notifications/dispatch/smtptest/server.go @@ -5,6 +5,7 @@ import ( _ "embed" "io" "net" + "slices" "sync" "time" @@ -53,8 +54,14 @@ func (b *Backend) NewSession(c *smtp.Conn) (smtp.Session, error) { return &Session{conn: c, backend: b}, nil } +// LastMessage returns a copy of the last message received by the +// backend. func (b *Backend) LastMessage() *Message { - return b.lastMsg + b.mu.Lock() + defer b.mu.Unlock() + clone := *b.lastMsg + clone.To = slices.Clone(b.lastMsg.To) + return &clone } func (b *Backend) Reset() { @@ -84,6 +91,9 @@ func (s *Session) Auth(mech string) (sasl.Server, error) { switch mech { case sasl.Plain: return sasl.NewPlainServer(func(identity, username, password string) error { + s.backend.mu.Lock() + defer s.backend.mu.Unlock() + s.backend.lastMsg.Identity = identity s.backend.lastMsg.Username = username s.backend.lastMsg.Password = password @@ -102,6 +112,9 @@ func (s *Session) Auth(mech string) (sasl.Server, error) { }), nil case sasl.Login: return sasl.NewLoginServer(func(username, password string) error { + s.backend.mu.Lock() + defer s.backend.mu.Unlock() + s.backend.lastMsg.Username = username s.backend.lastMsg.Password = password diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 86ed14fe90957..dacf4a1f7156c 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -1253,12 +1253,12 @@ func TestNotificationTemplates_Golden(t *testing.T) { // Spin up the mock webhook server var body []byte var readErr error - var webhookReceived bool + webhookReceived := make(chan struct{}) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) body, readErr = io.ReadAll(r.Body) - webhookReceived = true + close(webhookReceived) })) t.Cleanup(server.Close) @@ -1302,12 +1302,11 @@ func TestNotificationTemplates_Golden(t *testing.T) { ) require.NoError(t, err) - require.Eventually(t, func() bool { - return webhookReceived - }, testutil.WaitShort, testutil.IntervalFast) - - require.NoError(t, err) - + select { + case <-time.After(testutil.WaitShort): + require.Fail(t, "timed out waiting for webhook to be received") + case <-webhookReceived: + } // Handle the body that was read in the http server here. // We need to do it here because we can't call require.* in a separate goroutine, such as the http server handler require.NoError(t, readErr) From c2139ba17d95d8e1a99b9164ae2771dd96d9316b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 31 Oct 2024 15:30:10 +0200 Subject: [PATCH 2/3] nil check --- coderd/notifications/dispatch/smtptest/server.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/coderd/notifications/dispatch/smtptest/server.go b/coderd/notifications/dispatch/smtptest/server.go index 1aac9ecd55cfb..4b2e808677ce7 100644 --- a/coderd/notifications/dispatch/smtptest/server.go +++ b/coderd/notifications/dispatch/smtptest/server.go @@ -59,6 +59,9 @@ func (b *Backend) NewSession(c *smtp.Conn) (smtp.Session, error) { func (b *Backend) LastMessage() *Message { b.mu.Lock() defer b.mu.Unlock() + if b.lastMsg == nil { + return nil + } clone := *b.lastMsg clone.To = slices.Clone(b.lastMsg.To) return &clone From 2e5ca7a0ace66f82e615177bd18341359ba95ee2 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 31 Oct 2024 15:37:44 +0200 Subject: [PATCH 3/3] protect Reset while we are at it --- coderd/notifications/dispatch/smtptest/server.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/notifications/dispatch/smtptest/server.go b/coderd/notifications/dispatch/smtptest/server.go index 4b2e808677ce7..deb0d672604dc 100644 --- a/coderd/notifications/dispatch/smtptest/server.go +++ b/coderd/notifications/dispatch/smtptest/server.go @@ -68,6 +68,8 @@ func (b *Backend) LastMessage() *Message { } func (b *Backend) Reset() { + b.mu.Lock() + defer b.mu.Unlock() b.lastMsg = nil }