Linters clean-up, fix DNS mock leak in integration tests

foxcpp · foxcpp · commit 7d94d7772fec · 2026-03-03T17:45:56.000+03:00
diff --git a/framework/module/lifetime.go b/framework/module/lifetime.go
@@ -60,7 +60,9 @@ func (lt *LifetimeTracker) StartAll() error {
 		}
 
 		if err := entry.mod.Start(); err != nil {
-			lt.StopAll()
+			if err := lt.StopAll(); err != nil {
+				lt.logger.Error("StopAll failed after Start fail", err)
+			}
 			return fmt.Errorf("failed to start module %v: %w",
 				entry.mod.InstanceName(), err)
 		}
diff --git a/framework/resource/netresource/listen.go b/framework/resource/netresource/listen.go
@@ -16,8 +16,8 @@ func CloseUnusedListeners() error {
 	return tracker.CloseUnused()
 }
 
-func CloseAllListeners() {
-	tracker.Close()
+func CloseAllListeners() error {
+	return tracker.Close()
 }
 
 func ResetListenersUsage() {
diff --git a/framework/resource/netresource/tracker.go b/framework/resource/netresource/tracker.go
@@ -67,14 +67,22 @@ func (lt *ListenerTracker) ResetUsage() {
 }
 
 func (lt *ListenerTracker) CloseUnused() error {
-	lt.tcp.CloseUnused(func(key string) bool { return true })
-	lt.unix.CloseUnused(func(key string) bool { return true })
+	if err := lt.tcp.CloseUnused(func(key string) bool { return true }); err != nil {
+		lt.logger.Error("CloseUnused for TCP failed", err)
+	}
+	if err := lt.unix.CloseUnused(func(key string) bool { return true }); err != nil {
+		lt.logger.Error("CloseUnused for Unix failed", err)
+	}
 	return nil
 }
 
 func (lt *ListenerTracker) Close() error {
-	lt.tcp.Close()
-	lt.unix.Close()
+	if err := lt.tcp.Close(); err != nil {
+		lt.logger.Error("Close for TCP failed", err)
+	}
+	if err := lt.unix.Close(); err != nil {
+		lt.logger.Error("Close for Unix failed", err)
+	}
 	return nil
 }
 
diff --git a/go.mod b/go.mod
@@ -52,6 +52,7 @@ require (
 	github.com/minio/minio-go/v7 v7.0.84
 	github.com/netauth/netauth v0.6.2
 	github.com/prometheus/client_golang v1.20.5
+	github.com/stretchr/testify v1.10.0
 	github.com/urfave/cli/v2 v2.27.5
 	go.uber.org/zap v1.27.0
 	golang.org/x/crypto v0.32.0
@@ -87,6 +88,7 @@ require (
 	github.com/caddyserver/zerossl v0.1.3 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/cpuguy83/go-md2man/v2 v2.0.6 // indirect
+	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
 	github.com/digitalocean/godo v1.134.0 // indirect
 	github.com/dustin/go-humanize v1.0.1 // indirect
 	github.com/fatih/color v1.18.0 // indirect
@@ -122,6 +124,7 @@ require (
 	github.com/pelletier/go-toml/v2 v2.2.3 // indirect
 	github.com/pierrec/lz4 v2.6.1+incompatible // indirect
 	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
 	github.com/prometheus/client_model v0.6.1 // indirect
 	github.com/prometheus/common v0.62.0 // indirect
 	github.com/prometheus/procfs v0.15.1 // indirect
diff --git a/internal/cli/ctl/moduleinit.go b/internal/cli/ctl/moduleinit.go
@@ -21,19 +21,21 @@ package ctl
 import (
 	"errors"
 	"fmt"
-	"io"
 	"os"
 
 	"github.com/foxcpp/maddy"
 	"github.com/foxcpp/maddy/framework/container"
+	"github.com/foxcpp/maddy/framework/log"
 	"github.com/foxcpp/maddy/framework/module"
 	"github.com/foxcpp/maddy/internal/updatepipe"
 	"github.com/urfave/cli/v2"
 )
 
-func closeIfNeeded(i interface{}) {
-	if c, ok := i.(io.Closer); ok {
-		c.Close()
+func closeIfNeeded(i any) {
+	if c, ok := i.(module.LifetimeModule); ok {
+		if err := c.Stop(); err != nil {
+			log.DefaultLogger.Error("failed to stop module", err)
+		}
 	}
 }
 
diff --git a/internal/endpoint/imap/imap.go b/internal/endpoint/imap/imap.go
@@ -167,7 +167,9 @@ func (endp *Endpoint) Start() error {
 	}
 
 	if err := endp.setupListeners(endp.endpoints); err != nil {
-		endp.Stop()
+		if err := endp.Stop(); err != nil {
+			endp.Log.Error("failed to stop after setupListeners error", err)
+		}
 		return err
 	}
 	return nil
diff --git a/internal/endpoint/openmetrics/om.go b/internal/endpoint/openmetrics/om.go
@@ -85,7 +85,9 @@ func (e *Endpoint) Start() error {
 	for _, endp := range e.endpoints {
 		l, err := netresource.Listen(endp.Network(), endp.Address())
 		if err != nil {
-			e.Stop()
+			if err := e.Stop(); err != nil {
+
+			}
 			return fmt.Errorf("%s: %v", modName, err)
 		}
 
diff --git a/internal/endpoint/smtp/smtp.go b/internal/endpoint/smtp/smtp.go
@@ -318,7 +318,9 @@ func (endp *Endpoint) setConfig(cfg *config.Map) error {
 
 func (endp *Endpoint) Start() error {
 	if err := endp.setupListeners(endp.endpoints); err != nil {
-		endp.Stop()
+		if err := endp.Stop(); err != nil {
+			endp.Log.Error("failed to Stop after setupListeners fail", err)
+		}
 		return err
 	}
 	return nil
@@ -426,7 +428,9 @@ func (endp *Endpoint) Stop() error {
 	ctx, cancel := context.WithTimeout(context.Background(), endp.shutdownTimeout)
 	defer cancel()
 
-	endp.serv.Shutdown(ctx)
+	if err := endp.serv.Shutdown(ctx); err != nil {
+		return err
+	}
 
 	endp.listenersWg.Wait()
 
diff --git a/internal/endpoint/smtp/smtp_test.go b/internal/endpoint/smtp/smtp_test.go
@@ -37,6 +37,7 @@ import (
 	"github.com/foxcpp/maddy/internal/auth"
 	"github.com/foxcpp/maddy/internal/msgpipeline"
 	"github.com/foxcpp/maddy/internal/testutils"
+	"github.com/stretchr/testify/assert"
 )
 
 var testPort string
@@ -146,7 +147,9 @@ func submitMsgOpts(t *testing.T, cl *smtp.Client, from string, rcpts []string, o
 func TestSMTPDelivery(t *testing.T) {
 	tgt := testutils.Target{}
 	endp := testEndpoint(t, "smtp", nil, &tgt, nil, nil)
-	defer endp.Stop()
+	defer func() {
+		assert.NoError(t, endp.Stop())
+	}()
 
 	cl, err := smtp.Dial("127.0.0.1:" + testPort)
 	if err != nil {
diff --git a/internal/table/file_test.go b/internal/table/file_test.go
@@ -26,6 +26,7 @@ import (
 
 	"github.com/foxcpp/maddy/framework/config"
 	"github.com/foxcpp/maddy/internal/testutils"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestReadFile(t *testing.T) {
@@ -107,7 +108,9 @@ func TestFileReload(t *testing.T) {
 		t.Fatal(err)
 	}
 	m.log = testutils.Logger(t, "file_map")
-	defer m.Stop()
+	defer func() {
+		assert.NoError(t, m.Stop())
+	}()
 
 	if err := mod.Configure([]string{f.Name()}, &config.Map{Block: config.Node{}}); err != nil {
 		t.Fatal(err)
@@ -147,10 +150,10 @@ func TestFileReload_Broken(t *testing.T) {
 	}
 	defer os.Remove(f.Name())
 	if _, err := f.WriteString(file); err != nil {
-		f.Close()
+		assert.NoError(t, f.Close())
 		t.Fatal(err)
 	}
-	f.Close()
+	assert.NoError(t, f.Close())
 
 	mod, err := NewFile("", "")
 	if err != nil {
diff --git a/internal/target/queue/queue_test.go b/internal/target/queue/queue_test.go
@@ -39,6 +39,7 @@ import (
 	"github.com/foxcpp/maddy/framework/log"
 	"github.com/foxcpp/maddy/framework/module"
 	"github.com/foxcpp/maddy/internal/testutils"
+	"github.com/stretchr/testify/assert"
 )
 
 // newTestQueue returns properly initialized Queue object usable for testing.
@@ -269,7 +270,7 @@ func TestQueueDelivery_PermanentFail_NonPartial(t *testing.T) {
 
 	// Queue will abort a delivery if it fails for all recipients.
 	readMsgChanTimeout(t, dt.aborted, 5*time.Second)
-	q.Stop()
+	assert.NoError(t, q.Stop())
 
 	// Delivery is failed permanently, hence no retry should be rescheduled.
 	checkQueueDir(t, q, []string{})
@@ -296,7 +297,7 @@ func TestQueueDelivery_PermanentFail_Partial(t *testing.T) {
 	// Here delivery fails for recipients too, but this is reported using PartialDelivery.
 
 	readMsgChanTimeout(t, dt.aborted, 5*time.Second)
-	q.Stop()
+	assert.NoError(t, q.Stop())
 	checkQueueDir(t, q, []string{})
 }
 
diff --git a/internal/target/remote/mxauth_test.go b/internal/target/remote/mxauth_test.go
@@ -32,6 +32,7 @@ import (
 	"github.com/foxcpp/maddy/framework/dns"
 	"github.com/foxcpp/maddy/framework/module"
 	"github.com/foxcpp/maddy/internal/testutils"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestRemoteDelivery_AuthMX_MTASTS(t *testing.T) {
@@ -63,7 +64,9 @@ func TestRemoteDelivery_AuthMX_MTASTS(t *testing.T) {
 		testSTSPolicy(t, zones, mtastsGet),
 	})
 	tgt.tlsConfig = clientCfg
-	defer tgt.Stop()
+	defer func() {
+		assert.NoError(t, tgt.Stop())
+	}()
 
 	testutils.DoTestDelivery(t, tgt, "test@example.com", []string{"test@example.invalid"})
 	be.CheckMsg(t, 0, "test@example.com", []string{"test@example.invalid"})
@@ -108,7 +111,9 @@ func TestRemoteDelivery_MTASTS_SkipNonMatching(t *testing.T) {
 		&localPolicy{minMXLevel: module.MX_MTASTS},
 	})
 	tgt.tlsConfig = clientCfg
-	defer tgt.Stop()
+	defer func() {
+		assert.NoError(t, tgt.Stop())
+	}()
 
 	testutils.DoTestDelivery(t, tgt, "test@example.com", []string{"test@example.invalid"})
 	be.CheckMsg(t, 0, "test@example.com", []string{"test@example.invalid"})
@@ -148,7 +153,9 @@ func TestRemoteDelivery_AuthMX_MTASTS_Fail(t *testing.T) {
 		&localPolicy{minMXLevel: module.MX_MTASTS},
 	})
 	tgt.tlsConfig = clientCfg
-	defer tgt.Stop()
+	defer func() {
+		assert.NoError(t, tgt.Stop())
+	}()
 
 	_, err := testutils.DoTestDeliveryErr(t, tgt, "test@example.com", []string{"test@example.invalid"})
 	if err == nil {
diff --git a/maddy.go b/maddy.go
@@ -207,7 +207,11 @@ func Run(c *cli.Context) error {
 	defer log.DefaultLogger.Out.Close()
 	defer hooks.RunHooks(hooks.EventShutdown)
 
-	hooks.AddHook(hooks.EventShutdown, netresource.CloseAllListeners)
+	defer func() {
+		if err := netresource.CloseAllListeners(); err != nil {
+			log.DefaultLogger.Error("CloseAllListeners failed", err)
+		}
+	}()
 
 	if err := moduleMain(c.Path("config")); err != nil {
 		systemdStatusErr(err)
@@ -373,8 +377,8 @@ func moduleStart(c *container.C) error {
 	return c.Lifetime.StartAll()
 }
 
-func moduleStop(c *container.C) {
-	c.Lifetime.StopAll()
+func moduleStop(c *container.C) error {
+	return c.Lifetime.StopAll()
 }
 
 func moduleMain(configPath string) error {
@@ -412,7 +416,9 @@ func moduleMain(configPath string) error {
 	asyncStopWg.Wait()
 
 	systemdStatus(SDStopping, "Waiting for current configuration to stop...")
-	moduleStop(c)
+	if err := moduleStop(c); err != nil {
+		c.DefaultLogger.Msg("moduleStop failed", err)
+	}
 	c.DefaultLogger.Msg("server stopped")
 
 	return nil
@@ -446,10 +452,16 @@ func moduleReload(oldContainer *container.C, configPath string, asyncStopWg *syn
 	asyncStopWg.Add(1)
 	go func() {
 		defer asyncStopWg.Done()
-		defer netresource.CloseUnusedListeners()
+		defer func() {
+			if err := netresource.CloseUnusedListeners(); err != nil {
+				oldContainer.DefaultLogger.Error("CloseUnusedListeners failed", err)
+			}
+		}()
 
 		oldContainer.DefaultLogger.Msg("stopping old server")
-		moduleStop(oldContainer)
+		if err := moduleStop(oldContainer); err != nil {
+			oldContainer.DefaultLogger.Error("moduleStop failed", err)
+		}
 		oldContainer.DefaultLogger.Msg("old server stopped")
 
 		systemdStatus(SDReloading, "Configuration running.")
diff --git a/tests/t.go b/tests/t.go
@@ -101,12 +101,25 @@ func (t *T) DNS(zones map[string]mockdns.Zone) {
 		t.dnsServ.Close()
 	}
 
-	dnsServ, err := mockdns.NewServer(zones, false)
+	dnsServ, err := mockdns.NewServerWithLogger(zones, t, false)
 	if err != nil {
 		t.Fatal("Test configuration failed:", err)
 	}
 	dnsServ.Log = t
 	t.dnsServ = dnsServ
+
+	t.Cleanup(func() {
+		if t.dnsServ == nil {
+			return
+		}
+
+		// Shutdown the DNS server after maddy to make sure it will not spend time
+		// timing out queries.
+		if err := t.dnsServ.Close(); err != nil {
+			t.Log("Unable to stop the DNS server:", err)
+		}
+		t.dnsServ = nil
+	})
 }
 
 // Port allocates the random TCP port for use by test. It will made accessible
@@ -141,15 +154,6 @@ func (t *T) ensureCanRun() {
 		// any DNS queries to the real world.
 		t.Log("NOTE: Explicit DNS(nil) is recommended.")
 		t.DNS(nil)
-
-		t.Cleanup(func() {
-			// Shutdown the DNS server after maddy to make sure it will not spend time
-			// timing out queries.
-			if err := t.dnsServ.Close(); err != nil {
-				t.Log("Unable to stop the DNS server:", err)
-			}
-			t.dnsServ = nil
-		})
 	}
 
 	// Setup file system, create statedir, runtimedir, write out config.

Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,9 @@ func (lt *LifetimeTracker) StartAll() error {`
`60`	`60`	`}`
`61`	`61`
`62`	`62`	`if err := entry.mod.Start(); err != nil {`
`63`		`- lt.StopAll()`
	`63`	`+ if err := lt.StopAll(); err != nil {`
	`64`	`+ lt.logger.Error("StopAll failed after Start fail", err)`
	`65`	`+ }`
`64`	`66`	`return fmt.Errorf("failed to start module %v: %w",`
`65`	`67`	`entry.mod.InstanceName(), err)`
`66`	`68`	`}`
Original file line number	Diff line number	Diff line change
`@@ -16,8 +16,8 @@ func CloseUnusedListeners() error {`
`16`	`16`	`return tracker.CloseUnused()`
`17`	`17`	`}`
`18`	`18`
`19`		`-func CloseAllListeners() {`
`20`		`- tracker.Close()`
	`19`	`+func CloseAllListeners() error {`
	`20`	`+ return tracker.Close()`
`21`	`21`	`}`
`22`	`22`
`23`	`23`	`func ResetListenersUsage() {`
Original file line number	Diff line number	Diff line change
`@@ -167,7 +167,9 @@ func (endp *Endpoint) Start() error {`
`167`	`167`	`}`
`168`	`168`
`169`	`169`	`if err := endp.setupListeners(endp.endpoints); err != nil {`
`170`		`- endp.Stop()`
	`170`	`+ if err := endp.Stop(); err != nil {`
	`171`	`+ endp.Log.Error("failed to stop after setupListeners error", err)`
	`172`	`+ }`
`171`	`173`	`return err`
`172`	`174`	`}`
`173`	`175`	`return nil`