fix: race conditions in check shutdown & oapi version

* fix: race conditions in check shutdown
* fix: set openapi version on build time
* refactor: check base naming

Signed-off-by: lvlcn-t <[email protected]>

Author: lvlcn-t

.github/workflows/test_unit.yml renamed to .github/workflows/test_go.yml

@@ -1,4 +1,4 @@
-name: Test - Unit
+name: Test Go
 
 on:
   push:
@@ -19,7 +19,7 @@ jobs:
         with:
           go-version-file: go.mod
 
-      - name: Test
+      - name: Run all go tests
         run: |
           go mod download
-          go test --race --count=1 --coverprofile cover.out -v ./...
+          go test -race -count=1 -coverprofile cover.out -v ./...

cmd/run.go

@@ -66,8 +66,8 @@ func NewCmdRun() *cobra.Command {
 
 // run is the entry point to start the sparrow
 func run() func(cmd *cobra.Command, args []string) error {
-	return func(cmd *cobra.Command, _ []string) error {
-		cfg := &config.Config{Version: cmd.Root().Version}
+	return func(_ *cobra.Command, _ []string) error {
+		cfg := &config.Config{}
 		err := viper.Unmarshal(cfg)
 		if err != nil {
 			return fmt.Errorf("failed to parse config: %w", err)

main.go

@@ -20,12 +20,17 @@ package main
 
 import (
 	"github.com/caas-team/sparrow/cmd"
+	"github.com/caas-team/sparrow/pkg"
 )
 
 // Version is the current version of sparrow
 // It is set at build time by using -ldflags "-X main.version=x.x.x"
 var version string
 
+func init() { //nolint:gochecknoinits // Required for version to be set on build
+	pkg.Version = version
+}
+
 func main() {
 	cmd.Execute(version)
 }

pkg/checks/base.go

@@ -61,13 +61,25 @@ type Check interface {
 	RemoveLabelledMetrics(target string) error
 }
 
-// CheckBase is a struct providing common fields used by implementations of the Check interface.
+// Base is a struct providing common fields and methods used by implementations of the [Check] interface.
 // It serves as a foundational structure that should be embedded in specific check implementations.
-type CheckBase struct {
+type Base struct {
 	// Mutex for thread-safe access to shared resources within the check implementation
 	Mu sync.Mutex
 	// Signal channel used to notify about shutdown of a check
 	DoneChan chan struct{}
+	// closed is a flag indicating if the check has been shut down.
+	closed bool
+}
+
+// Shutdown closes the DoneChan to signal the check to stop running.
+func (b *Base) Shutdown() {
+	b.Mu.Lock()
+	defer b.Mu.Unlock()
+	if !b.closed {
+		close(b.DoneChan)
+		b.closed = true
+	}
 }
 
 // Runtime is the interface that all check configurations must implement

pkg/checks/base_test.go

@@ -0,0 +1,44 @@
+package checks
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestBase_Shutdown(t *testing.T) {
+	tests := []struct {
+		name string
+		b    *Base
+	}{
+		{
+			name: "shutdown",
+			b: &Base{
+				DoneChan: make(chan struct{}, 1),
+			},
+		},
+		{
+			name: "already shutdown",
+			b: &Base{
+				DoneChan: make(chan struct{}, 1),
+				closed:   true,
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.b.closed {
+				close(tt.b.DoneChan)
+			}
+			tt.b.Shutdown()
+
+			if !tt.b.closed {
+				t.Error("Base.Shutdown() should close DoneChan")
+			}
+
+			assert.Panics(t, func() {
+				tt.b.DoneChan <- struct{}{}
+			}, "Base.DoneChan should be closed")
+		})
+	}
+}

pkg/checks/dns/dns.go

@@ -41,7 +41,7 @@ const CheckName = "dns"
 
 // DNS is a check that resolves the names and addresses
 type DNS struct {
-	checks.CheckBase
+	checks.Base
 	config  Config
 	metrics metrics
 	client  Resolver
@@ -60,7 +60,7 @@ func (d *DNS) Name() string {
 // NewCheck creates a new instance of the dns check
 func NewCheck() checks.Check {
 	return &DNS{
-		CheckBase: checks.CheckBase{
+		Base: checks.Base{
 			Mu:       sync.Mutex{},
 			DoneChan: make(chan struct{}, 1),
 		},
@@ -108,11 +108,6 @@ func (d *DNS) Run(ctx context.Context, cResult chan checks.ResultDTO) error {
 	}
 }
 
-func (d *DNS) Shutdown() {
-	d.DoneChan <- struct{}{}
-	close(d.DoneChan)
-}
-
 func (d *DNS) UpdateConfig(cfg checks.Runtime) error {
 	if c, ok := cfg.(*Config); ok {
 		d.Mu.Lock()

pkg/checks/dns/dns_test.go

@@ -51,7 +51,7 @@ func TestDNS_Run(t *testing.T) {
 			name: "success with no targets",
 			mockSetup: func() *DNS {
 				return &DNS{
-					CheckBase: checks.CheckBase{
+					Base: checks.Base{
 						Mu:       sync.Mutex{},
 						DoneChan: make(chan struct{}, 1),
 					},
@@ -241,21 +241,6 @@ func TestDNS_Run_Context_Done(t *testing.T) {
 	time.Sleep(time.Millisecond * 30)
 }
 
-func TestDNS_Shutdown(t *testing.T) {
-	cDone := make(chan struct{}, 1)
-	c := DNS{
-		CheckBase: checks.CheckBase{
-			DoneChan: cDone,
-		},
-	}
-	c.Shutdown()
-
-	_, ok := <-cDone
-	if !ok {
-		t.Error("Shutdown() should be ok")
-	}
-}
-
 func TestDNS_UpdateConfig(t *testing.T) {
 	tests := []struct {
 		name    string
@@ -322,7 +307,7 @@ func stringPointer(s string) *string {
 
 func newCommonDNS() *DNS {
 	return &DNS{
-		CheckBase: checks.CheckBase{
+		Base: checks.Base{
 			Mu:       sync.Mutex{},
 			DoneChan: make(chan struct{}, 1),
 		},

pkg/checks/health/health.go

@@ -48,15 +48,15 @@ const CheckName = "health"
 
 // Health is a check that measures the availability of an endpoint
 type Health struct {
-	checks.CheckBase
+	checks.Base
 	config  Config
 	metrics metrics
 }
 
 // NewCheck creates a new instance of the health check
 func NewCheck() checks.Check {
 	return &Health{
-		CheckBase: checks.CheckBase{
+		Base: checks.Base{
 			Mu:       sync.Mutex{},
 			DoneChan: make(chan struct{}, 1),
 		},
@@ -97,12 +97,6 @@ func (h *Health) Run(ctx context.Context, cResult chan checks.ResultDTO) error {
 	}
 }
 
-// Shutdown is called once when the check is unregistered or sparrow shuts down
-func (h *Health) Shutdown() {
-	h.DoneChan <- struct{}{}
-	close(h.DoneChan)
-}
-
 // UpdateConfig sets the configuration for the health check
 func (h *Health) UpdateConfig(cfg checks.Runtime) error {
 	if c, ok := cfg.(*Config); ok {

pkg/checks/health/health_test.go

@@ -247,33 +247,3 @@ func TestHealth_Check(t *testing.T) {
 		})
 	}
 }
-
-func TestHealth_Shutdown(t *testing.T) {
-	cDone := make(chan struct{}, 1)
-	c := Health{
-		CheckBase: checks.CheckBase{
-			DoneChan: cDone,
-		},
-	}
-	c.Shutdown()
-
-	if _, ok := <-cDone; !ok {
-		t.Error("Channel should be done")
-	}
-
-	assert.Panics(t, func() {
-		cDone <- struct{}{}
-	}, "Channel is closed, should panic")
-
-	hc := NewCheck()
-	hc.Shutdown()
-
-	_, ok := <-hc.(*Health).DoneChan
-	if !ok {
-		t.Error("Channel should be done")
-	}
-
-	assert.Panics(t, func() {
-		hc.(*Health).DoneChan <- struct{}{}
-	}, "Channel is closed, should panic")
-}

pkg/checks/latency/latency.go

@@ -43,15 +43,15 @@ const CheckName = "latency"
 
 // Latency is a check that measures the latency to an endpoint
 type Latency struct {
-	checks.CheckBase
+	checks.Base
 	config  Config
 	metrics metrics
 }
 
 // NewCheck creates a new instance of the latency check
 func NewCheck() checks.Check {
 	return &Latency{
-		CheckBase: checks.CheckBase{
+		Base: checks.Base{
 			Mu:       sync.Mutex{},
 			DoneChan: make(chan struct{}, 1),
 		},
@@ -98,11 +98,6 @@ func (l *Latency) Run(ctx context.Context, cResult chan checks.ResultDTO) error
 	}
 }
 
-func (l *Latency) Shutdown() {
-	l.DoneChan <- struct{}{}
-	close(l.DoneChan)
-}
-
 // UpdateConfig sets the configuration for the latency check
 func (l *Latency) UpdateConfig(cfg checks.Runtime) error {
 	if c, ok := cfg.(*Config); ok {

pkg/checks/latency/latency_test.go

@@ -305,21 +305,6 @@ func TestLatency_check(t *testing.T) {
 	}
 }
 
-func TestLatency_Shutdown(t *testing.T) {
-	cDone := make(chan struct{}, 1)
-	c := Latency{
-		CheckBase: checks.CheckBase{
-			DoneChan: cDone,
-		},
-	}
-	c.Shutdown()
-
-	_, ok := <-cDone
-	if !ok {
-		t.Error("Shutdown() should be ok")
-	}
-}
-
 func TestLatency_UpdateConfig(t *testing.T) {
 	c := Latency{}
 	wantCfg := Config{

pkg/checks/traceroute/check.go

@@ -53,7 +53,7 @@ func (t Target) String() string {
 
 func NewCheck() checks.Check {
 	c := &Traceroute{
-		CheckBase: checks.CheckBase{
+		Base: checks.Base{
 			Mu:       sync.Mutex{},
 			DoneChan: make(chan struct{}, 1),
 		},
@@ -66,7 +66,7 @@ func NewCheck() checks.Check {
 }
 
 type Traceroute struct {
-	checks.CheckBase
+	checks.Base
 	config     Config
 	traceroute tracerouteFactory
 	metrics    metrics
@@ -212,12 +212,6 @@ func (tr *Traceroute) check(ctx context.Context) map[string]result {
 	return res
 }
 
-// Shutdown is called once when the check is unregistered or sparrow shuts down
-func (tr *Traceroute) Shutdown() {
-	tr.DoneChan <- struct{}{}
-	close(tr.DoneChan)
-}
-
 // UpdateConfig is called once when the check is registered
 // This is also called while the check is running, if the remote config is updated
 // This should return an error if the config is invalid

pkg/checks/traceroute/check_test.go

@@ -79,7 +79,7 @@ func newForTest(f tracerouteFactory, maxHops int, targets []string) *Traceroute
 		t[i] = Target{Addr: target}
 	}
 	return &Traceroute{
-		CheckBase:  checks.CheckBase{Mu: sync.Mutex{}, DoneChan: make(chan struct{})},
+		Base:       checks.Base{Mu: sync.Mutex{}, DoneChan: make(chan struct{})},
 		config:     Config{Targets: t, MaxHops: maxHops},
 		traceroute: f,
 		metrics:    newMetrics(),

pkg/config/config.go

@@ -29,10 +29,6 @@ import (
 )
 
 type Config struct {
-	// Version is the version of the sparrow.
-	// This is set at build time by using -ldflags "-X main.version=x.x.x"
-	// and is not part of the configuration file or flags.
-	Version string `yaml:"-" mapstructure:"-"`
 	// SparrowName is the DNS name of the sparrow
 	SparrowName string `yaml:"name" mapstructure:"name"`
 	// Loader is the configuration for the loader