From 88948f1e7dd388638cba35e888a8da667bad6934 Mon Sep 17 00:00:00 2001 From: butterrobot Date: Mon, 7 Sep 2026 16:28:11 +0000 Subject: [PATCH 1/4] deps: upgrade Go to 1.27.1, refresh dependencies and drop gotoolkit Replaces the gotoolkit helpers with the standard library and the libraries they wrapped: - gotoolkit/encoding TOML wrapper -> pelletier/go-toml/v2 directly, behind a new smtp2shoutrrr.LoadConfig shared by both commands. - gotoolkit/service + gotoolkit/model.Server -> signal.NotifyContext and a concrete *Server whose Start shuts the listener down gracefully when its context is cancelled. Also drops the unused ANONYMOUS SASL client from cmd/sendmail, bumps alpine, golangci-lint and the CI actions, and adds coverage for config loading and the server lifecycle. Closes FMG-2 Co-authored-by: multica-agent Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 20 ++++----- .github/workflows/release.yml | 6 +-- Containerfile | 2 +- Makefile | 2 +- cmd/sendmail/main.go | 78 ++++++++++------------------------ cmd/smtp2shoutrrr/main.go | 50 +++++++--------------- config.go | 22 ++++++++++ config_test.go | 78 ++++++++++++++++++++++++++++++++++ go.mod | 37 +++++++--------- go.sum | 79 +++++++++++++---------------------- server.go | 45 ++++++++++++++------ server_test.go | 64 ++++++++++++++++++++++++++++ 12 files changed, 293 insertions(+), 190 deletions(-) create mode 100644 config_test.go create mode 100644 server_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9e13b63..867ff91 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,8 +11,8 @@ jobs: runs-on: docker container: git.nakama.town/fmartingr/ci-images/ci-base:1.0.0 steps: - - uses: actions/checkout@v6 - - uses: actions/setup-go@v6 + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 with: go-version-file: go.mod - run: make format @@ -22,8 +22,8 @@ jobs: runs-on: docker container: git.nakama.town/fmartingr/ci-images/ci-base:1.0.0 steps: - - uses: actions/checkout@v6 - - uses: actions/goreleaser-action@v6.4.0 + - uses: actions/checkout@v7 + - uses: actions/goreleaser-action@v7.2.3 with: args: check @@ -31,8 +31,8 @@ jobs: runs-on: docker container: git.nakama.town/fmartingr/ci-images/ci-base:1.0.0 steps: - - uses: actions/checkout@v6 - - uses: actions/setup-go@v6 + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 with: go-version-file: go.mod - run: make ci-lint @@ -41,8 +41,8 @@ jobs: runs-on: docker container: git.nakama.town/fmartingr/ci-images/ci-base:1.0.0 steps: - - uses: actions/checkout@v6 - - uses: actions/setup-go@v6 + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 with: go-version-file: go.mod - run: make test @@ -51,8 +51,8 @@ jobs: runs-on: docker container: git.nakama.town/fmartingr/ci-images/ci-base:1.0.0 steps: - - uses: actions/checkout@v6 - - uses: actions/setup-go@v6 + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 with: go-version-file: go.mod - run: make build diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1f41606..7fed7b2 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -9,12 +9,12 @@ jobs: runs-on: docker container: git.nakama.town/fmartingr/ci-images/ci-base:1.0.0 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 - name: Install Go - uses: actions/setup-go@v6 + uses: actions/setup-go@v7 with: go-version-file: go.mod @@ -22,7 +22,7 @@ jobs: run: echo "${{ secrets.FORGEJO_TOKEN }}" | docker login git.nakama.town -u ${{ github.actor }} --password-stdin - name: Run GoReleaser - uses: actions/goreleaser-action@v6.4.0 + uses: actions/goreleaser-action@v7.2.3 with: args: release --clean env: diff --git a/Containerfile b/Containerfile index 4472a6e..0526ef5 100644 --- a/Containerfile +++ b/Containerfile @@ -1,5 +1,5 @@ # Build dependencies on native arch (no QEMU needed) -FROM --platform=$BUILDPLATFORM alpine:3.20 AS base +FROM --platform=$BUILDPLATFORM alpine:3.24 AS base RUN apk add --no-cache ca-certificates tzdata RUN addgroup -g 1000 smtp2shoutrrr && adduser -u 1000 -G smtp2shoutrrr -s /bin/sh -D smtp2shoutrrr diff --git a/Makefile b/Makefile index 879fad3..99c70c5 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ CGO_ENABLED := 0 DIST_PATH := ./dist TEST_OPTIONS := -v -failfast -race -bench=. -benchtime=100000x -cover -coverprofile=coverage.out -GOLANGCI_LINT_VERSION := v2.8.0 +GOLANGCI_LINT_VERSION := v2.13.2 .DEFAULT_GOAL := help diff --git a/cmd/sendmail/main.go b/cmd/sendmail/main.go index df894b3..5ce37cf 100644 --- a/cmd/sendmail/main.go +++ b/cmd/sendmail/main.go @@ -1,41 +1,15 @@ package main import ( + "errors" "fmt" - "log" "log/slog" "net/smtp" "os" - "git.nakama.town/fmartingr/gotoolkit/encoding" - "github.com/emersion/go-sasl" - "git.nakama.town/fmartingr/smtp2shoutrrr" ) -// The ANONYMOUS mechanism name. -const Anonymous = "ANONYMOUS" - -type anonymousClient struct { - Trace string -} - -func (c *anonymousClient) Start(si *smtp.ServerInfo) (mech string, ir []byte, err error) { - mech = Anonymous - ir = []byte(c.Trace) - return -} - -func (c *anonymousClient) Next(challenge []byte, b bool) (response []byte, err error) { - return nil, sasl.ErrUnexpectedServerChallenge -} - -// A client implementation of the ANONYMOUS authentication mechanism, as -// described in RFC 4505. -func NewAnonymousClient(trace string) smtp.Auth { - return &anonymousClient{trace} -} - // The PLAIN mechanism name. const Plain = "PLAIN" @@ -46,63 +20,57 @@ type plainClient struct { } func (a *plainClient) Start(si *smtp.ServerInfo) (mech string, ir []byte, err error) { - mech = "PLAIN" + mech = Plain ir = []byte(a.Identity + "\x00" + a.Username + "\x00" + a.Password) return } func (a *plainClient) Next(challenge []byte, b bool) (response []byte, err error) { - slog.Info("Next: %v", slog.String("challenge", string(challenge))) + slog.Debug("SASL challenge received", slog.String("challenge", string(challenge))) return nil, nil } -// A client implementation of the PLAIN authentication mechanism, as described -// in RFC 4616. Authorization identity may be left blank to indicate that it is -// the same as the username. +// NewPlainClient is a client implementation of the PLAIN authentication +// mechanism, as described in RFC 4616. Unlike smtp.PlainAuth it does not +// require a TLS connection, which the development server does not offer. +// Authorization identity may be left blank to indicate that it is the same as +// the username. func NewPlainClient(identity, username, password string) smtp.Auth { return &plainClient{identity, username, password} } +const configPath = "config.toml" + func main() { - var config smtp2shoutrrr.Config - configPath := "config.toml" + if err := run(); err != nil { + slog.Error("sendmail failed", slog.String("err", err.Error())) + os.Exit(1) + } +} - f, err := os.Open(configPath) +func run() error { + config, err := smtp2shoutrrr.LoadConfig(configPath) if err != nil { - slog.Error("Error opening config file", slog.String("err", err.Error()), slog.String("path", configPath)) - return + return fmt.Errorf("loading configuration: %w", err) } - enc := encoding.NewTOMLEncoding() - if err := enc.DecodeReader(f, &config); err != nil { - slog.Error("Error decoding config file", slog.String("err", err.Error()), slog.String("path", configPath)) - return - } - - config.SetDefaults() - - // hostname is used by PlainAuth to validate the TLS certificate. + // The development server listens on localhost without TLS. hostname := "localhost" auth := NewPlainClient("", config.Username, config.Password) - // auth := NewAnonymousClient("test") slog.Info("Using first recipient configuration to send a test email") if len(config.Recipients) == 0 { - slog.Error("No recipients found in configuration") - return + return errors.New("no recipients found in configuration") } if len(config.Recipients[0].Addresses) == 0 { - slog.Error("No email addresses found in first recipient configuration") - return + return errors.New("no email addresses found in first recipient configuration") } recipients := []string{config.Recipients[0].Addresses[0]} msg := []byte("Subject: Test notification\r\n\r\nThis is a test notification") from := "hello@localhost" - err = smtp.SendMail(fmt.Sprintf("%s:%d", hostname, config.Port), auth, from, recipients, msg) - if err != nil { - log.Fatal(err) - } + + return smtp.SendMail(fmt.Sprintf("%s:%d", hostname, config.Port), auth, from, recipients, msg) } diff --git a/cmd/smtp2shoutrrr/main.go b/cmd/smtp2shoutrrr/main.go index 0d92081..b8b8585 100644 --- a/cmd/smtp2shoutrrr/main.go +++ b/cmd/smtp2shoutrrr/main.go @@ -2,54 +2,36 @@ package main import ( "context" + "fmt" "log/slog" "os" + "os/signal" + "syscall" - "git.nakama.town/fmartingr/gotoolkit/encoding" - "git.nakama.town/fmartingr/gotoolkit/model" - "git.nakama.town/fmartingr/gotoolkit/service" "git.nakama.town/fmartingr/smtp2shoutrrr" _ "golang.org/x/crypto/x509roots/fallback" ) +const configPath = "config.toml" + func main() { - var config *smtp2shoutrrr.Config - configPath := "config.toml" + if err := run(); err != nil { + slog.Error("smtp2shoutrrr stopped", slog.String("err", err.Error())) + os.Exit(1) + } +} - f, err := os.Open(configPath) +func run() error { + config, err := smtp2shoutrrr.LoadConfig(configPath) if err != nil { - slog.Error("Error opening config file", slog.String("err", err.Error()), slog.String("path", configPath)) - return + return fmt.Errorf("loading configuration: %w", err) } - enc := encoding.NewTOMLEncoding() - if err := enc.DecodeReader(f, &config); err != nil { - slog.Error("Error decoding config file", slog.String("err", err.Error()), slog.String("path", configPath)) - return - } - - config.SetDefaults() - slog.Info("config loaded", slog.Int("recipients", len(config.Recipients))) - ctx := context.Background() - smtpServer := smtp2shoutrrr.NewSMTPServer(config) + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop() - svc, err := service.NewService([]model.Server{ - smtpServer, - }) - if err != nil { - slog.Error("Error creating service:", slog.String("err", err.Error())) - return - } - - if err := svc.Start(ctx); err != nil { - slog.Error("Error starting service:", slog.String("err", err.Error())) - return - } - - if err := svc.WaitStop(ctx); err != nil { - slog.Error("Error waiting for service interruption:", slog.String("err", err.Error())) - } + return smtp2shoutrrr.NewSMTPServer(config).Start(ctx) } diff --git a/config.go b/config.go index 232fd41..6009221 100644 --- a/config.go +++ b/config.go @@ -1,11 +1,33 @@ package smtp2shoutrrr import ( + "fmt" "log/slog" "net/url" + "os" "strings" + + "github.com/pelletier/go-toml/v2" ) +// LoadConfig reads the TOML configuration at path and applies its defaults. +func LoadConfig(path string) (*Config, error) { + f, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("opening config file %q: %w", path, err) + } + defer func() { _ = f.Close() }() + + var config Config + if err := toml.NewDecoder(f).Decode(&config); err != nil { + return nil, fmt.Errorf("decoding config file %q: %w", path, err) + } + + config.SetDefaults() + + return &config, nil +} + type Config struct { Port int Username string diff --git a/config_test.go b/config_test.go new file mode 100644 index 0000000..994f782 --- /dev/null +++ b/config_test.go @@ -0,0 +1,78 @@ +package smtp2shoutrrr + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func writeConfig(t *testing.T, contents string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "config.toml") + require.NoError(t, os.WriteFile(path, []byte(contents), 0o600)) + + return path +} + +func TestLoadConfig(t *testing.T) { + path := writeConfig(t, ` +Port = 2525 +Username = "user" +Password = "secret" + +[[Recipients]] +Addresses = ["user@example.com"] +Targets = ["ntfy://ntfy.sh/topic"] + +[[Recipients]] +Addresses = ["legacy@example.com"] +Target = "ntfy://ntfy.sh/legacy" + +[CatchAll] +Targets = ["ntfy://ntfy.sh/catch-all"] +`) + + config, err := LoadConfig(path) + require.NoError(t, err) + + require.Equal(t, 2525, config.Port) + require.Equal(t, "user", config.Username) + require.Equal(t, "secret", config.Password) + + require.Len(t, config.Recipients, 2) + require.Equal(t, []string{"user@example.com"}, config.Recipients[0].Addresses) + require.Equal(t, []string{"ntfy://ntfy.sh/topic"}, config.Recipients[0].Targets) + require.Equal(t, "ntfy://ntfy.sh/legacy", config.Recipients[1].Target) + + require.NotNil(t, config.CatchAll) + require.Equal(t, []string{"ntfy://ntfy.sh/catch-all"}, config.CatchAll.Targets) +} + +func TestLoadConfigAppliesDefaults(t *testing.T) { + path := writeConfig(t, ` +[[Recipients]] +Addresses = ["user@example.com"] +Targets = ["ntfy://ntfy.sh/topic"] +`) + + config, err := LoadConfig(path) + require.NoError(t, err) + + require.Equal(t, 11125, config.Port) + require.Equal(t, "username", config.Username) + require.Equal(t, "password", config.Password) + require.Nil(t, config.CatchAll) +} + +func TestLoadConfigMissingFile(t *testing.T) { + _, err := LoadConfig(filepath.Join(t.TempDir(), "does-not-exist.toml")) + require.Error(t, err) +} + +func TestLoadConfigInvalidTOML(t *testing.T) { + _, err := LoadConfig(writeConfig(t, "Port = ")) + require.Error(t, err) +} diff --git a/go.mod b/go.mod index d04bc12..957b483 100644 --- a/go.mod +++ b/go.mod @@ -1,34 +1,25 @@ module git.nakama.town/fmartingr/smtp2shoutrrr -go 1.25.6 +go 1.27.1 require ( - git.nakama.town/fmartingr/gotoolkit v0.2.4 github.com/containrrr/shoutrrr v0.8.0 github.com/emersion/go-sasl v0.0.0-20241020182733-b788ff22d5a6 - github.com/emersion/go-smtp v0.24.0 - github.com/stretchr/testify v1.9.0 - golang.org/x/crypto/x509roots/fallback v0.0.0-20250214233241-911360c8a4f4 + github.com/emersion/go-smtp v0.25.0 + github.com/pelletier/go-toml/v2 v2.4.3 + github.com/stretchr/testify v1.12.1 + golang.org/x/crypto/x509roots/fallback v0.0.0-20260902180247-86efde54dc70 ) require ( - github.com/davecgh/go-spew v1.1.1 // indirect - github.com/fatih/color v1.18.0 // indirect - github.com/go-logr/logr v1.4.1 // indirect + github.com/fatih/color v1.19.0 // indirect + github.com/go-logr/logr v1.4.4 // indirect github.com/golang/protobuf v1.5.4 // indirect - github.com/google/go-cmp v0.6.0 // indirect - github.com/kr/pretty v0.3.0 // indirect - github.com/mattn/go-colorable v0.1.14 // indirect - github.com/mattn/go-isatty v0.0.20 // indirect - github.com/pelletier/go-toml/v2 v2.2.3 // indirect - github.com/pmezard/go-difflib v1.0.0 // indirect - github.com/rogpeppe/go-internal v1.8.1 // indirect - golang.org/x/net v0.25.0 // indirect - golang.org/x/sys v0.40.0 // indirect - golang.org/x/text v0.21.0 // indirect - golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d // indirect - gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect + github.com/google/go-cmp v0.7.0 // indirect + github.com/mattn/go-colorable v0.1.15 // indirect + github.com/mattn/go-isatty v0.0.24 // indirect + go.yaml.in/yaml/v3 v3.0.5 // indirect + golang.org/x/net v0.58.0 // indirect + golang.org/x/sys v0.47.0 // indirect + golang.org/x/tools v0.49.0 // indirect ) - -//replace git.nakama.town/fmartingr/gotoolkit => ../gotoolkit diff --git a/go.sum b/go.sum index f92faca..5b3ff23 100644 --- a/go.sum +++ b/go.sum @@ -1,71 +1,48 @@ -git.nakama.town/fmartingr/gotoolkit v0.2.4 h1:mnq15OXzHdtTjr4Ows5WMqQfqHraAnRwIhhuOtuqBlU= -git.nakama.town/fmartingr/gotoolkit v0.2.4/go.mod h1:Ak68T/qEx0xwhB/9hzE+cbQRTAiWre3Tj1VdTK9gKyo= github.com/containrrr/shoutrrr v0.8.0 h1:mfG2ATzIS7NR2Ec6XL+xyoHzN97H8WPjir8aYzJUSec= github.com/containrrr/shoutrrr v0.8.0/go.mod h1:ioyQAyu1LJY6sILuNyKaQaw+9Ttik5QePU8atnAdO2o= -github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= -github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= -github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/emersion/go-sasl v0.0.0-20241020182733-b788ff22d5a6 h1:oP4q0fw+fOSWn3DfFi4EXdT+B+gTtzx8GC9xsc26Znk= github.com/emersion/go-sasl v0.0.0-20241020182733-b788ff22d5a6/go.mod h1:iL2twTeMvZnrg54ZoPDNfJaJaqy0xIQFuBdrLsmspwQ= -github.com/emersion/go-smtp v0.24.0 h1:g6AfoF140mvW0vLNPD/LuCBLEAdlxOjIXqbIkJIS6Wk= -github.com/emersion/go-smtp v0.24.0/go.mod h1:ZtRRkbTyp2XTHCA+BmyTFTrj8xY4I+b4McvHxCU2gsQ= -github.com/fatih/color v1.18.0 h1:S8gINlzdQ840/4pfAwic/ZE0djQEH3wM94VfqLTZcOM= -github.com/fatih/color v1.18.0/go.mod h1:4FelSpRwEGDpQ12mAdzqdOukCy4u8WUtOY6lkT/6HfU= -github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ= -github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/emersion/go-smtp v0.25.0 h1:krfiHrme2JbJYDh0DGuSRbvPpbnQTH/v9CIfPincl1I= +github.com/emersion/go-smtp v0.25.0/go.mod h1:ZtRRkbTyp2XTHCA+BmyTFTrj8xY4I+b4McvHxCU2gsQ= +github.com/fatih/color v1.19.0 h1:Zp3PiM21/9Ld6FzSKyL5c/BULoe/ONr9KlbYVOfG8+w= +github.com/fatih/color v1.19.0/go.mod h1:zNk67I0ZUT1bEGsSGyCZYZNrHuTkJJB+r6Q9VuMi0LE= +github.com/go-logr/logr v1.4.4 h1:tG4xh9yMsRCAiodLVTxyrkzSZ9+o0L1Kg/+cPVcbP/8= +github.com/go-logr/logr v1.4.4/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572 h1:tfuBGBXKqDEevZMzYi5KSi8KkcZtzBcTgAUUtapy0OI= github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572/go.mod h1:9Pwr4B2jHnOSGXyyzV8ROjYa2ojvAY6HCGYYfMoC3Ls= github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= -github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= -github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/google/pprof v0.0.0-20210407192527-94a9f03dee38 h1:yAJXTCF9TqKcTiHJAE8dj7HMvPfh66eeA2JYW7eFpSE= github.com/google/pprof v0.0.0-20210407192527-94a9f03dee38/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= github.com/jarcoal/httpmock v1.3.0 h1:2RJ8GP0IIaWwcC9Fp2BmVi8Kog3v2Hn7VXM3fTd+nuc= github.com/jarcoal/httpmock v1.3.0/go.mod h1:3yb8rc4BI7TCBhFY8ng0gjuLKJNquuDNiPaZjnENuYg= -github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= -github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= -github.com/kr/pretty v0.3.0 h1:WgNl7dwNpEZ6jJ9k1snq4pZsg7DOEN8hP9Xw0Tsjwk0= -github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk= -github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= -github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= -github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= -github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= -github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHPsaIE= -github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8= -github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= -github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= +github.com/mattn/go-colorable v0.1.15 h1:+u9SLTRGnXv73cEsnsmoZBom+dMU88B2M0aDcWy0/jY= +github.com/mattn/go-colorable v0.1.15/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8= +github.com/mattn/go-isatty v0.0.24 h1:tGZZoVgT/KiqK1c8ocVLeDS8BSWMRd47J3Lbz7vsReI= +github.com/mattn/go-isatty v0.0.24/go.mod h1:nMCL3Zebbrt45jsMDgnfIwz6ydEQApk5oEI3HqDio6A= github.com/onsi/ginkgo/v2 v2.9.2 h1:BA2GMJOtfGAfagzYtrAlufIP0lq6QERkFmHLMLPwFSU= github.com/onsi/ginkgo/v2 v2.9.2/go.mod h1:WHcJJG2dIlcCqVfBAwUCrJxSPFb6v4azBwgxeMeDuts= github.com/onsi/gomega v1.27.6 h1:ENqfyGeS5AX/rlXDd/ETokDz93u0YufY1Pgxuy/PvWE= github.com/onsi/gomega v1.27.6/go.mod h1:PIQNjfQwkP3aQAH7lf7j87O/5FiNr+ZR8+ipb+qQlhg= -github.com/pelletier/go-toml/v2 v2.2.3 h1:YmeHyLY8mFWbdkNWwpr+qIL2bEqT0o95WSdkNHvL12M= -github.com/pelletier/go-toml/v2 v2.2.3/go.mod h1:MfCQTFTvCcUyyvvwm1+G6H/jORL20Xlb6rzQu9GuUkc= -github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= -github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= -github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= -github.com/rogpeppe/go-internal v1.8.1 h1:geMPLpDpQOgVyCg5z5GoRwLHepNdb71NXb67XFkP+Eg= -github.com/rogpeppe/go-internal v1.8.1/go.mod h1:JeRgkft04UBgHMgCIwADu4Pn6Mtm5d4nPKWu0nJ5d+o= -github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= -github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= -golang.org/x/crypto/x509roots/fallback v0.0.0-20250214233241-911360c8a4f4 h1:QDiVWrFJ2lyXzr3pJnIREQWR8S7jkjzuWJPJda8Ic8E= -golang.org/x/crypto/x509roots/fallback v0.0.0-20250214233241-911360c8a4f4/go.mod h1:lxN5T34bK4Z/i6cMaU7frUU57VkDXFD4Kamfl/cp9oU= -golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac= -golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= -golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.40.0 h1:DBZZqJ2Rkml6QMQsZywtnjnnGvHza6BTfYFWY9kjEWQ= -golang.org/x/sys v0.40.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= -golang.org/x/text v0.21.0 h1:zyQAAkrwaneQ066sspRyJaG9VNi/YJ1NfzcGB3hZ/qo= -golang.org/x/text v0.21.0/go.mod h1:4IBbMaMmOPCJ8SecivzSH54+73PCFmPWxNTLm+vZkEQ= -golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d h1:vU5i/LfpvrRCpgM/VPfJLg5KjxD3E+hfT1SH+d9zLwg= -golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d/go.mod h1:aiJjzUbINMkxbQROHiO6hDPo2LHcIPhhQsa9DLh0yGk= +github.com/pelletier/go-toml/v2 v2.4.3 h1:GTRvJQutkOSftxIFD5xw9aepkYNuPWmVJpffdDPYVpY= +github.com/pelletier/go-toml/v2 v2.4.3/go.mod h1:2gIqNv+qfxSVS7cM2xJQKtLSTLUE9V8t9Stt+h56mCY= +github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE= +github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg= +go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= +go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= +golang.org/x/crypto/x509roots/fallback v0.0.0-20260902180247-86efde54dc70 h1:VwViOGcd7C8/Gs18efVlMxvUCS0un6KRKBPFEBvpUXg= +golang.org/x/crypto/x509roots/fallback v0.0.0-20260902180247-86efde54dc70/go.mod h1:HPze8vhfG6fO06AM+VSvxRm4E3+5Yk375mgrJ5M2z1E= +golang.org/x/net v0.58.0 h1:ynWG7rqYi4ccpTEuPZ2QGWHktVEM9DMCj9yzDE0Q7To= +golang.org/x/net v0.58.0/go.mod h1:YwCddHnFlT7eLQqVprV19OnhLGtc5xOKgE0RyqgfWAU= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/text v0.41.0 h1:vz/seA0lnX87Othu2f/0L24RcgrXD9/YFTSuGjj3rH8= +golang.org/x/text v0.41.0/go.mod h1:jvf1O8ajNzZqhSrQBPbutR/EB83Cc0CFrezNQIwbb5M= +golang.org/x/tools v0.49.0 h1:3NI7VXzL9+1WZD52Dx2ttoPwD5DWrFGpl9mFZDlmisI= +golang.org/x/tools v0.49.0/go.mod h1:SJNXV9DBKT0UbdttsQjbfJlAE/q+y36++zo3uL3N0Oo= google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI= google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos= -gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= -gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= -gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/server.go b/server.go index 7262576..5a0fa4b 100644 --- a/server.go +++ b/server.go @@ -2,35 +2,56 @@ package smtp2shoutrrr import ( "context" + "errors" "fmt" "log/slog" "time" - "git.nakama.town/fmartingr/gotoolkit/model" "github.com/emersion/go-smtp" ) -var _ model.Server = (*smtpServer)(nil) +// shutdownTimeout bounds how long in-flight sessions are given to finish once +// the server is asked to stop. +const shutdownTimeout = 10 * time.Second -type smtpServer struct { +type Server struct { backend *smtp.Server } -func (s *smtpServer) IsEnabled() bool { - return true -} - -func (s *smtpServer) Start(_ context.Context) error { +// Start accepts connections until ctx is cancelled or Stop is called, then +// shuts the listener down gracefully. It returns nil on a clean shutdown. +func (s *Server) Start(ctx context.Context) error { slog.Info("Started SMTP server", slog.String("addr", s.backend.Addr)) - return s.backend.ListenAndServe() + + served := make(chan error, 1) + go func() { + served <- s.backend.ListenAndServe() + }() + + select { + case err := <-served: + return err + case <-ctx.Done(): + } + + // The incoming context is already cancelled, so the shutdown deadline has + // to be derived from a live one. + shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), shutdownTimeout) + defer cancel() + + if err := s.Stop(shutdownCtx); err != nil && !errors.Is(err, smtp.ErrServerClosed) { + return err + } + + return <-served } -func (s *smtpServer) Stop(ctx context.Context) error { +func (s *Server) Stop(ctx context.Context) error { slog.Info("Stopping SMTP server") return s.backend.Shutdown(ctx) } -func NewSMTPServer(config *Config) model.Server { +func NewSMTPServer(config *Config) *Server { be := &Backend{ config: config, } @@ -43,7 +64,7 @@ func NewSMTPServer(config *Config) model.Server { smtpBackend.MaxRecipients = 50 smtpBackend.AllowInsecureAuth = true - return &smtpServer{ + return &Server{ backend: smtpBackend, } } diff --git a/server_test.go b/server_test.go new file mode 100644 index 0000000..c678d5f --- /dev/null +++ b/server_test.go @@ -0,0 +1,64 @@ +package smtp2shoutrrr + +import ( + "context" + "fmt" + "net" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func dialSMTP(t *testing.T, port int) (net.Conn, error) { + t.Helper() + + return net.DialTimeout("tcp", fmt.Sprintf("localhost:%d", port), 200*time.Millisecond) +} + +func TestServerShutsDownOnContextCancel(t *testing.T) { + config := &Config{Port: 2530, Username: "testuser", Password: "testpass"} + server := NewSMTPServer(config) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + stopped := make(chan error, 1) + go func() { + stopped <- server.Start(ctx) + }() + + require.Eventually(t, func() bool { + conn, err := dialSMTP(t, config.Port) + if err != nil { + return false + } + require.NoError(t, conn.Close()) + return true + }, 5*time.Second, 20*time.Millisecond, "server never started listening") + + cancel() + + select { + case err := <-stopped: + require.NoError(t, err) + case <-time.After(shutdownTimeout + time.Second): + t.Fatal("server did not shut down after context cancellation") + } + + _, err := dialSMTP(t, config.Port) + require.Error(t, err, "listener should be released once Start returns") +} + +func TestServerStartReturnsListenError(t *testing.T) { + config := &Config{Port: 2531, Username: "testuser", Password: "testpass"} + + blocker, err := net.Listen("tcp", fmt.Sprintf(":%d", config.Port)) + require.NoError(t, err) + t.Cleanup(func() { _ = blocker.Close() }) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancel) + + require.Error(t, NewSMTPServer(config).Start(ctx)) +} -- 2.52.0 From 953d033f462696ecca0e1af38e540027ff1e60e3 Mon Sep 17 00:00:00 2001 From: butterrobot Date: Mon, 7 Sep 2026 16:54:09 +0000 Subject: [PATCH 2/4] ci: use the goreleaser from the CI image instead of goreleaser-action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit goreleaser-action v7.1.0 made cosign signature verification of its own download mandatory, and ci-base ships no cosign — the goreleaser-lint job failed on the v6.4.0 -> v7.2.3 bump. The action was redundant anyway: ci-base already installs goreleaser from the upstream apt repo, which is what `make build` has always used. Both workflows now call it directly, and `goreleaser check` moves behind a `make check` target so every CI job runs a make target. Co-authored-by: multica-agent Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 4 +--- .github/workflows/release.yml | 4 +--- Makefile | 5 +++++ 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 867ff91..bdc9527 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,9 +23,7 @@ jobs: container: git.nakama.town/fmartingr/ci-images/ci-base:1.0.0 steps: - uses: actions/checkout@v7 - - uses: actions/goreleaser-action@v7.2.3 - with: - args: check + - run: make check lint: runs-on: docker diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 7fed7b2..1e3ab84 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -22,9 +22,7 @@ jobs: run: echo "${{ secrets.FORGEJO_TOKEN }}" | docker login git.nakama.town -u ${{ github.actor }} --password-stdin - name: Run GoReleaser - uses: actions/goreleaser-action@v7.2.3 - with: - args: release --clean + run: goreleaser release --clean env: GORELEASER_FORCE_TOKEN: gitea GITEA_TOKEN: ${{ secrets.FORGEJO_TOKEN }} diff --git a/Makefile b/Makefile index 99c70c5..5cecb03 100644 --- a/Makefile +++ b/Makefile @@ -35,6 +35,11 @@ format: go fmt ./... go mod tidy +## check: Validate the goreleaser configuration +.PHONY: check +check: + goreleaser check + ## ci-lint: Run golangci-lint (installs if missing) .PHONY: ci-lint ci-lint: -- 2.52.0 From 91b5e7b77e4a02fcfe3b6fc55830fae1a170534c Mon Sep 17 00:00:00 2001 From: butterrobot Date: Mon, 7 Sep 2026 18:30:10 +0000 Subject: [PATCH 3/4] fix: address review findings on the server lifecycle and config loading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Start bound the listener inside ListenAndServe on a goroutine, so a stop that won the race found go-smtp's listener list still empty, closed nothing, and left Accept running forever — a SIGTERM during startup needed SIGKILL. Bind synchronously and serve the listener we own, and close it from Stop so a direct caller gets the same guarantee. shutdownTimeout equalled ReadTimeout, so draining an idle client (which only drops once its read deadline expires) raced its own budget and surfaced an intentional stop as "context deadline exceeded", which main turned into a non-zero exit. Give the drain room and log an overrun instead of failing. Also: - Validate config after decoding. TOML ignores unknown keys, so [[Recipient]] or Adresses previously started a server that answered 250 OK and forwarded nothing. - Drop cmd/sendmail's hand-rolled PLAIN client; net/smtp.PlainAuth already permits plaintext to a loopback host, as backend_test.go relies on. - Release the signal handler once the first signal lands, so a second Ctrl-C aborts a slow drain. - go.mod: express the language version, not a patch pin, so consumers on go1.27.0 with GOTOOLCHAIN=local still build. - Makefile: install goreleaser like golangci-lint, so check/build work on a clean checkout. - Tests: ephemeral ports throughout, no require inside an Eventually condition, and regression coverage for both lifecycle bugs. Co-Authored-By: Claude Opus 5 (1M context) Co-authored-by: multica-agent --- Makefile | 9 ++- README.md | 5 ++ backend_test.go | 105 +++++------------------------- cmd/sendmail/main.go | 41 +++--------- cmd/smtp2shoutrrr/main.go | 7 ++ config.go | 35 +++++++++- config_test.go | 48 ++++++++++++++ go.mod | 4 +- server.go | 82 ++++++++++++++++++++---- server_test.go | 131 ++++++++++++++++++++++++++++++++------ 10 files changed, 307 insertions(+), 160 deletions(-) diff --git a/Makefile b/Makefile index 5cecb03..c5b7d47 100644 --- a/Makefile +++ b/Makefile @@ -5,6 +5,7 @@ DIST_PATH := ./dist TEST_OPTIONS := -v -failfast -race -bench=. -benchtime=100000x -cover -coverprofile=coverage.out GOLANGCI_LINT_VERSION := v2.13.2 +GORELEASER_VERSION := v2.18.1 .DEFAULT_GOAL := help @@ -14,9 +15,13 @@ help: @echo "Available targets:" @grep -E '^## ' $(MAKEFILE_LIST) | sed 's/## / /' | sort +.PHONY: ensure-goreleaser +ensure-goreleaser: + @which goreleaser > /dev/null 2>&1 || go install github.com/goreleaser/goreleaser/v2@$(GORELEASER_VERSION) + ## build: Build project using goreleaser (snapshot) .PHONY: build -build: +build: ensure-goreleaser goreleaser build --snapshot --clean ## quick-run: Execute project directly using go run @@ -37,7 +42,7 @@ format: ## check: Validate the goreleaser configuration .PHONY: check -check: +check: ensure-goreleaser goreleaser check ## ci-lint: Run golangci-lint (installs if missing) diff --git a/README.md b/README.md index ad3a5d8..3666932 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,11 @@ Target = "ntfy://ntfy.sh/legacy-topic?tags=thing" # Will show deprecation warni Targets = ["ntfy://ntfy.sh/catch-all-topic?tags=unmatched"] ``` +The server refuses to start if the configuration cannot forward anything — no +recipients and no catch-all, a recipient without addresses or usable targets, or +a mistyped table name such as `[[Recipient]]`, which TOML would otherwise accept +silently. + ### From releases - Grab the latest release from the [releases page](https://git.nakama.town/fmartingr/smtp2shoutrrr/releases) diff --git a/backend_test.go b/backend_test.go index d9c5cc2..330519c 100644 --- a/backend_test.go +++ b/backend_test.go @@ -1,8 +1,6 @@ package smtp2shoutrrr import ( - "context" - "fmt" "io" "net/http" "net/http/httptest" @@ -36,7 +34,7 @@ func TestEmailForwarding(t *testing.T) { // Configure the SMTP server config := &Config{ - Port: 2525, + Port: 0, Username: "testuser", Password: "testpass", Recipients: []ConfigRecipient{ @@ -47,28 +45,12 @@ func TestEmailForwarding(t *testing.T) { }, } - ctx := context.Background() - smtpServer := NewSMTPServer(config) - - // Start the server - go func() { - if err := smtpServer.Start(ctx); err != nil { - t.Errorf("failed to start server: %v", err) - } - }() - - // Give the server time to start - time.Sleep(100 * time.Millisecond) - defer func() { - if err := smtpServer.Stop(ctx); err != nil { - t.Errorf("failed to stop server: %v", err) - } - }() + addr, _, _ := startServer(t, NewSMTPServer(config)) // Send test email auth := smtp.PlainAuth("", config.Username, config.Password, "localhost") err := smtp.SendMail( - fmt.Sprintf("localhost:%d", config.Port), + addr, auth, "sender@example.com", []string{"test@example.com"}, @@ -76,7 +58,6 @@ func TestEmailForwarding(t *testing.T) { ) require.NoError(t, err) - // Give some time for the notification to be processed time.Sleep(100 * time.Millisecond) // Verify the notification was received @@ -106,7 +87,7 @@ func TestMultipleTargets(t *testing.T) { // Configure with multiple targets config := &Config{ - Port: 2526, + Port: 0, Username: "testuser", Password: "testpass", Recipients: []ConfigRecipient{ @@ -120,26 +101,12 @@ func TestMultipleTargets(t *testing.T) { }, } - ctx := context.Background() - smtpServer := NewSMTPServer(config) - - go func() { - if err := smtpServer.Start(ctx); err != nil { - t.Errorf("failed to start server: %v", err) - } - }() - - time.Sleep(100 * time.Millisecond) - defer func() { - if err := smtpServer.Stop(ctx); err != nil { - t.Errorf("failed to stop server: %v", err) - } - }() + addr, _, _ := startServer(t, NewSMTPServer(config)) // Send test email auth := smtp.PlainAuth("", config.Username, config.Password, "localhost") err := smtp.SendMail( - fmt.Sprintf("localhost:%d", config.Port), + addr, auth, "sender@example.com", []string{"test@example.com"}, @@ -172,7 +139,7 @@ func TestTargetAndTargetsMerging(t *testing.T) { // Configure with both Target (deprecated) and Targets config := &Config{ - Port: 2527, + Port: 0, Username: "testuser", Password: "testpass", Recipients: []ConfigRecipient{ @@ -187,26 +154,12 @@ func TestTargetAndTargetsMerging(t *testing.T) { }, } - ctx := context.Background() - smtpServer := NewSMTPServer(config) - - go func() { - if err := smtpServer.Start(ctx); err != nil { - t.Errorf("failed to start server: %v", err) - } - }() - - time.Sleep(100 * time.Millisecond) - defer func() { - if err := smtpServer.Stop(ctx); err != nil { - t.Errorf("failed to stop server: %v", err) - } - }() + addr, _, _ := startServer(t, NewSMTPServer(config)) // Send test email auth := smtp.PlainAuth("", config.Username, config.Password, "localhost") err := smtp.SendMail( - fmt.Sprintf("localhost:%d", config.Port), + addr, auth, "sender@example.com", []string{"test@example.com"}, @@ -244,7 +197,7 @@ func TestPartialTargetFailure(t *testing.T) { // Configure with mix of good and bad targets config := &Config{ - Port: 2528, + Port: 0, Username: "testuser", Password: "testpass", Recipients: []ConfigRecipient{ @@ -259,26 +212,12 @@ func TestPartialTargetFailure(t *testing.T) { }, } - ctx := context.Background() - smtpServer := NewSMTPServer(config) - - go func() { - if err := smtpServer.Start(ctx); err != nil { - t.Errorf("failed to start server: %v", err) - } - }() - - time.Sleep(100 * time.Millisecond) - defer func() { - if err := smtpServer.Stop(ctx); err != nil { - t.Errorf("failed to stop server: %v", err) - } - }() + addr, _, _ := startServer(t, NewSMTPServer(config)) // Send test email auth := smtp.PlainAuth("", config.Username, config.Password, "localhost") err := smtp.SendMail( - fmt.Sprintf("localhost:%d", config.Port), + addr, auth, "sender@example.com", []string{"test@example.com"}, @@ -309,7 +248,7 @@ func TestInvalidTargetURLs(t *testing.T) { // Configure with invalid and valid URLs config := &Config{ - Port: 2529, + Port: 0, Username: "testuser", Password: "testpass", Recipients: []ConfigRecipient{ @@ -325,26 +264,12 @@ func TestInvalidTargetURLs(t *testing.T) { }, } - ctx := context.Background() - smtpServer := NewSMTPServer(config) - - go func() { - if err := smtpServer.Start(ctx); err != nil { - t.Errorf("failed to start server: %v", err) - } - }() - - time.Sleep(100 * time.Millisecond) - defer func() { - if err := smtpServer.Stop(ctx); err != nil { - t.Errorf("failed to stop server: %v", err) - } - }() + addr, _, _ := startServer(t, NewSMTPServer(config)) // Send test email auth := smtp.PlainAuth("", config.Username, config.Password, "localhost") err := smtp.SendMail( - fmt.Sprintf("localhost:%d", config.Port), + addr, auth, "sender@example.com", []string{"test@example.com"}, diff --git a/cmd/sendmail/main.go b/cmd/sendmail/main.go index 5ce37cf..ede4b78 100644 --- a/cmd/sendmail/main.go +++ b/cmd/sendmail/main.go @@ -10,36 +10,14 @@ import ( "git.nakama.town/fmartingr/smtp2shoutrrr" ) -// The PLAIN mechanism name. -const Plain = "PLAIN" +const ( + configPath = "config.toml" -type plainClient struct { - Identity string - Username string - Password string -} - -func (a *plainClient) Start(si *smtp.ServerInfo) (mech string, ir []byte, err error) { - mech = Plain - ir = []byte(a.Identity + "\x00" + a.Username + "\x00" + a.Password) - return -} - -func (a *plainClient) Next(challenge []byte, b bool) (response []byte, err error) { - slog.Debug("SASL challenge received", slog.String("challenge", string(challenge))) - return nil, nil -} - -// NewPlainClient is a client implementation of the PLAIN authentication -// mechanism, as described in RFC 4616. Unlike smtp.PlainAuth it does not -// require a TLS connection, which the development server does not offer. -// Authorization identity may be left blank to indicate that it is the same as -// the username. -func NewPlainClient(identity, username, password string) smtp.Auth { - return &plainClient{identity, username, password} -} - -const configPath = "config.toml" + // PlainAuth refuses to send credentials over an unencrypted connection + // unless the host is a loopback address, which is what the development + // server listens on. + hostname = "localhost" +) func main() { if err := run(); err != nil { @@ -54,10 +32,6 @@ func run() error { return fmt.Errorf("loading configuration: %w", err) } - // The development server listens on localhost without TLS. - hostname := "localhost" - auth := NewPlainClient("", config.Username, config.Password) - slog.Info("Using first recipient configuration to send a test email") if len(config.Recipients) == 0 { @@ -68,6 +42,7 @@ func run() error { return errors.New("no email addresses found in first recipient configuration") } + auth := smtp.PlainAuth("", config.Username, config.Password, hostname) recipients := []string{config.Recipients[0].Addresses[0]} msg := []byte("Subject: Test notification\r\n\r\nThis is a test notification") from := "hello@localhost" diff --git a/cmd/smtp2shoutrrr/main.go b/cmd/smtp2shoutrrr/main.go index b8b8585..73ca2f1 100644 --- a/cmd/smtp2shoutrrr/main.go +++ b/cmd/smtp2shoutrrr/main.go @@ -33,5 +33,12 @@ func run() error { ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() + // Restore default signal handling as soon as the first signal arrives, so + // a second one aborts a drain instead of being swallowed. + go func() { + <-ctx.Done() + stop() + }() + return smtp2shoutrrr.NewSMTPServer(config).Start(ctx) } diff --git a/config.go b/config.go index 6009221..2696295 100644 --- a/config.go +++ b/config.go @@ -1,6 +1,7 @@ package smtp2shoutrrr import ( + "errors" "fmt" "log/slog" "net/url" @@ -10,7 +11,8 @@ import ( "github.com/pelletier/go-toml/v2" ) -// LoadConfig reads the TOML configuration at path and applies its defaults. +// LoadConfig reads the TOML configuration at path, applies its defaults and +// checks that it can actually deliver something. func LoadConfig(path string) (*Config, error) { f, err := os.Open(path) if err != nil { @@ -25,6 +27,10 @@ func LoadConfig(path string) (*Config, error) { config.SetDefaults() + if err := config.Validate(); err != nil { + return nil, fmt.Errorf("invalid config file %q: %w", path, err) + } + return &config, nil } @@ -52,6 +58,33 @@ func (c *Config) SetDefaults() { } } +// Validate rejects configuration that would leave the server accepting mail it +// can never forward. A TOML decode ignores unknown keys, so a mistyped table or +// field name ([[Recipient]], Adresses) otherwise yields a server that answers +// 250 OK to everything and sends nothing. +func (c *Config) Validate() error { + for i := range c.Recipients { + r := &c.Recipients[i] + if len(r.Addresses) == 0 { + return fmt.Errorf("recipient %d has no Addresses", i) + } + if len(r.GetTargetURLs()) == 0 { + return fmt.Errorf("recipient %d (%s) has no usable Targets", + i, strings.Join(r.Addresses, ",")) + } + } + + if c.CatchAll != nil && len(c.CatchAll.GetTargetURLs()) == 0 { + return errors.New("CatchAll has no usable Targets") + } + + if len(c.Recipients) == 0 && c.CatchAll == nil { + return errors.New("no Recipients and no CatchAll configured") + } + + return nil +} + type ConfigRecipient struct { Addresses []string // email addresses Target string // deprecated: use Targets instead diff --git a/config_test.go b/config_test.go index 994f782..686f048 100644 --- a/config_test.go +++ b/config_test.go @@ -76,3 +76,51 @@ func TestLoadConfigInvalidTOML(t *testing.T) { _, err := LoadConfig(writeConfig(t, "Port = ")) require.Error(t, err) } + +func TestLoadConfigRejectsUnusableConfigurations(t *testing.T) { + // A TOML decode ignores unknown keys, so each of these parses cleanly and + // would otherwise start a server that forwards nothing. + for name, contents := range map[string]string{ + "mistyped recipients table": ` +[[Recipient]] +Addresses = ["user@example.com"] +Targets = ["ntfy://ntfy.sh/topic"] +`, + "mistyped addresses key": ` +[[Recipients]] +Adresses = ["user@example.com"] +Targets = ["ntfy://ntfy.sh/topic"] +`, + "recipient without targets": ` +[[Recipients]] +Addresses = ["user@example.com"] +`, + "recipient with only unparseable targets": ` +[[Recipients]] +Addresses = ["user@example.com"] +Targets = ["://invalid-url"] +`, + "catch-all without targets": ` +[CatchAll] +Addresses = ["user@example.com"] +`, + "empty configuration": ` +Port = 2525 +`, + } { + t.Run(name, func(t *testing.T) { + _, err := LoadConfig(writeConfig(t, contents)) + require.Error(t, err) + }) + } +} + +func TestLoadConfigAcceptsCatchAllOnly(t *testing.T) { + config, err := LoadConfig(writeConfig(t, ` +[CatchAll] +Targets = ["ntfy://ntfy.sh/catch-all"] +`)) + require.NoError(t, err) + require.Empty(t, config.Recipients) + require.NotNil(t, config.CatchAll) +} diff --git a/go.mod b/go.mod index 957b483..0892ec1 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,8 @@ module git.nakama.town/fmartingr/smtp2shoutrrr -go 1.27.1 +go 1.27 + +toolchain go1.27.1 require ( github.com/containrrr/shoutrrr v0.8.0 diff --git a/server.go b/server.go index 5a0fa4b..20aacb5 100644 --- a/server.go +++ b/server.go @@ -5,27 +5,77 @@ import ( "errors" "fmt" "log/slog" + "net" + "sync" "time" "github.com/emersion/go-smtp" ) -// shutdownTimeout bounds how long in-flight sessions are given to finish once -// the server is asked to stop. -const shutdownTimeout = 10 * time.Second +const ( + readTimeout = 10 * time.Second + writeTimeout = 10 * time.Second + + // shutdownTimeout bounds the drain of in-flight sessions. It must stay + // clear of readTimeout: an idle client is only dropped once its read + // deadline expires, so a budget at or below readTimeout would report an + // ordinary drain as a failed shutdown. + shutdownTimeout = readTimeout + 20*time.Second +) type Server struct { backend *smtp.Server + + mu sync.Mutex + listener net.Listener + addr string } -// Start accepts connections until ctx is cancelled or Stop is called, then -// shuts the listener down gracefully. It returns nil on a clean shutdown. +// Addr reports the address the server is bound to, which is only known after +// Start has bound it — relevant when the configured port is 0. +func (s *Server) Addr() string { + s.mu.Lock() + defer s.mu.Unlock() + return s.addr +} + +func (s *Server) setListener(l net.Listener) { + s.mu.Lock() + defer s.mu.Unlock() + s.listener = l + s.addr = l.Addr().String() +} + +func (s *Server) closeListener() { + s.mu.Lock() + l := s.listener + s.listener = nil + s.mu.Unlock() + + if l != nil { + _ = l.Close() + } +} + +// Start binds the configured address and serves connections until ctx is +// cancelled or Stop is called, then drains in-flight sessions. It returns nil +// on a clean shutdown. func (s *Server) Start(ctx context.Context) error { - slog.Info("Started SMTP server", slog.String("addr", s.backend.Addr)) + // Binding here rather than inside ListenAndServe keeps the listener under + // our control: go-smtp's Shutdown only closes listeners Serve has already + // registered, so a stop that arrives first would otherwise leave Accept + // running forever. + l, err := net.Listen("tcp", s.backend.Addr) + if err != nil { + return fmt.Errorf("listen on %q: %w", s.backend.Addr, err) + } + s.setListener(l) + + slog.Info("Started SMTP server", slog.String("addr", l.Addr().String())) served := make(chan error, 1) go func() { - served <- s.backend.ListenAndServe() + served <- s.backend.Serve(l) }() select { @@ -34,13 +84,15 @@ func (s *Server) Start(ctx context.Context) error { case <-ctx.Done(): } - // The incoming context is already cancelled, so the shutdown deadline has - // to be derived from a live one. + // ctx is already cancelled, so the drain budget needs a live context. shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), shutdownTimeout) defer cancel() if err := s.Stop(shutdownCtx); err != nil && !errors.Is(err, smtp.ErrServerClosed) { - return err + // Outrunning the drain budget is still an intentional stop; reporting + // it as a failure would exit the process non-zero on a routine signal. + slog.Warn("SMTP server stopped with sessions still in flight", + slog.String("err", err.Error())) } return <-served @@ -48,7 +100,11 @@ func (s *Server) Start(ctx context.Context) error { func (s *Server) Stop(ctx context.Context) error { slog.Info("Stopping SMTP server") - return s.backend.Shutdown(ctx) + + err := s.backend.Shutdown(ctx) + s.closeListener() + + return err } func NewSMTPServer(config *Config) *Server { @@ -58,8 +114,8 @@ func NewSMTPServer(config *Config) *Server { smtpBackend := smtp.NewServer(be) smtpBackend.Addr = fmt.Sprintf(":%d", config.Port) - smtpBackend.WriteTimeout = 10 * time.Second - smtpBackend.ReadTimeout = 10 * time.Second + smtpBackend.WriteTimeout = writeTimeout + smtpBackend.ReadTimeout = readTimeout smtpBackend.MaxMessageBytes = 1024 * 1024 smtpBackend.MaxRecipients = 50 smtpBackend.AllowInsecureAuth = true diff --git a/server_test.go b/server_test.go index c678d5f..1dadc8d 100644 --- a/server_test.go +++ b/server_test.go @@ -2,7 +2,6 @@ package smtp2shoutrrr import ( "context" - "fmt" "net" "testing" "time" @@ -10,33 +9,59 @@ import ( "github.com/stretchr/testify/require" ) -func dialSMTP(t *testing.T, port int) (net.Conn, error) { - t.Helper() - - return net.DialTimeout("tcp", fmt.Sprintf("localhost:%d", port), 200*time.Millisecond) +// loopbackAddr rewrites the bound address to a dialable loopback address. +// The host stays "localhost" because smtp.PlainAuth refuses to authenticate +// when the dialed host differs from the one it was constructed with. It +// reports "" rather than failing the test, because it runs inside +// require.Eventually conditions, which execute on their own goroutine where +// t.FailNow would deadlock instead of reporting. +func loopbackAddr(s *Server) string { + _, port, err := net.SplitHostPort(s.Addr()) + if err != nil { + return "" + } + return net.JoinHostPort("localhost", port) } -func TestServerShutsDownOnContextCancel(t *testing.T) { - config := &Config{Port: 2530, Username: "testuser", Password: "testpass"} - server := NewSMTPServer(config) +func canDial(addr string) bool { + if addr == "" { + return false + } + conn, err := net.DialTimeout("tcp", addr, 200*time.Millisecond) + if err != nil { + return false + } + return conn.Close() == nil +} + +// startServer starts srv on an ephemeral port and blocks until it accepts +// connections, returning the dial address and the channel Start returns on. +func startServer(t *testing.T, srv *Server) (string, context.CancelFunc, <-chan error) { + t.Helper() ctx, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) stopped := make(chan error, 1) go func() { - stopped <- server.Start(ctx) + stopped <- srv.Start(ctx) }() require.Eventually(t, func() bool { - conn, err := dialSMTP(t, config.Port) - if err != nil { - return false - } - require.NoError(t, conn.Close()) - return true + return canDial(loopbackAddr(srv)) }, 5*time.Second, 20*time.Millisecond, "server never started listening") + return loopbackAddr(srv), cancel, stopped +} + +func testConfig() *Config { + return &Config{Port: 0, Username: "testuser", Password: "testpass"} +} + +func TestServerShutsDownOnContextCancel(t *testing.T) { + srv := NewSMTPServer(testConfig()) + addr, cancel, stopped := startServer(t, srv) + cancel() select { @@ -46,19 +71,85 @@ func TestServerShutsDownOnContextCancel(t *testing.T) { t.Fatal("server did not shut down after context cancellation") } - _, err := dialSMTP(t, config.Port) - require.Error(t, err, "listener should be released once Start returns") + require.False(t, canDial(addr), "listener should be released once Start returns") } func TestServerStartReturnsListenError(t *testing.T) { - config := &Config{Port: 2531, Username: "testuser", Password: "testpass"} - - blocker, err := net.Listen("tcp", fmt.Sprintf(":%d", config.Port)) + blocker, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) t.Cleanup(func() { _ = blocker.Close() }) + _, port, err := net.SplitHostPort(blocker.Addr().String()) + require.NoError(t, err) + + config := testConfig() + config.Port = mustAtoi(t, port) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) t.Cleanup(cancel) require.Error(t, NewSMTPServer(config).Start(ctx)) } + +// Regression: go-smtp's Shutdown only closes listeners Serve has registered. +// When the stop won that race, Accept kept running and Start never returned — +// a SIGTERM during startup left a process that only SIGKILL could clear. +func TestServerStartReturnsWhenCancelledBeforeListening(t *testing.T) { + srv := NewSMTPServer(testConfig()) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + stopped := make(chan error, 1) + go func() { + stopped <- srv.Start(ctx) + }() + + select { + case err := <-stopped: + require.NoError(t, err) + case <-time.After(10 * time.Second): + t.Fatal("Start did not return when its context was cancelled before the listener registered") + } + + require.False(t, canDial(loopbackAddr(srv)), "listener should not be left accepting connections") +} + +// Regression: an idle client is only dropped once its read deadline expires, +// so the drain lasts about ReadTimeout. With the shutdown budget set to the +// same value, an ordinary stop surfaced as "context deadline exceeded" and the +// command turned that into a non-zero exit. +func TestServerDrainsIdleConnectionWithoutError(t *testing.T) { + srv := NewSMTPServer(testConfig()) + srv.backend.ReadTimeout = 300 * time.Millisecond + + addr, cancel, stopped := startServer(t, srv) + + conn, err := net.DialTimeout("tcp", addr, time.Second) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + + cancel() + + select { + case err := <-stopped: + require.NoError(t, err, "an intentional shutdown must not report an error") + case <-time.After(shutdownTimeout + time.Second): + t.Fatal("server did not drain the idle connection") + } +} + +// The drain waits out a client's read deadline, so the budget has to exceed it +// or every stop with an idle connection reports a failure. +func TestShutdownBudgetExceedsReadTimeout(t *testing.T) { + require.Greater(t, shutdownTimeout, readTimeout) +} + +func mustAtoi(t *testing.T, s string) int { + t.Helper() + + n, err := net.LookupPort("tcp", s) + require.NoError(t, err) + + return n +} -- 2.52.0 From 45410d3de71b9d7b3e7734d5f1976c02f9fb3db4 Mon Sep 17 00:00:00 2001 From: butterrobot Date: Mon, 7 Sep 2026 19:00:58 +0000 Subject: [PATCH 4/4] fix: close the go-smtp drain race and pin the shutdown budget by behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit go-smtp registers each accepted session on a WaitGroup that Shutdown waits on from another goroutine, so an Accept still in flight when the drain begins is an Add concurrent with that Wait. Under -race, which is how make test runs, the suite failed 3/20 locally. Stop accepting and let Serve return before draining, which removes the overlap entirely: 0/25 with no data race reported. It also means a session accepted in that instant is now actually waited for. Closing the listener first makes Serve return net.ErrClosed and makes Shutdown's own close fail the same way, so both are filtered — without that, every clean shutdown logged a spurious "still in flight" warning. The timeout half of the earlier fix was pinned only by a constant assertion: a budget one millisecond over readTimeout, or reverting the warn back to a returned error, both left the whole suite green. The budget is now a Server field so a test can drive a real overrun, and TestServerReportsNoErrorWhenDrainOverrunsBudget asserts Start returns nil on it; the constant assertion now requires a 10s margin rather than a strict inequality. Both reverts fail these tests. Also: - Warn about config keys the decode ignored, instead of dropping them silently. A mistyped [[Recipient]] alongside a valid [CatchAll] used to start with recipients=0 and no signal at all. Still not fatal, so configurations carrying a stray key keep working. - Skip targets that parse but carry no scheme; url.Parse accepts almost any string, so "usable target" did not previously mean much. - Release the listener when Serve exits on its own, rather than leaking it to a library caller. - Document that a Server is single use. - Wait for the bind rather than a probe connection in tests, and report a Start failure instead of letting it look like a slow bind. Co-Authored-By: Claude Opus 5 (1M context) Co-authored-by: multica-agent --- config.go | 36 +++++++++++++++++++++++--- config_test.go | 30 ++++++++++++++++++++++ server.go | 54 +++++++++++++++++++++++++++++--------- server_test.go | 70 ++++++++++++++++++++++++++++++++++++++++++-------- 4 files changed, 164 insertions(+), 26 deletions(-) diff --git a/config.go b/config.go index 2696295..7fc4ffb 100644 --- a/config.go +++ b/config.go @@ -1,6 +1,7 @@ package smtp2shoutrrr import ( + "bytes" "errors" "fmt" "log/slog" @@ -14,17 +15,18 @@ import ( // LoadConfig reads the TOML configuration at path, applies its defaults and // checks that it can actually deliver something. func LoadConfig(path string) (*Config, error) { - f, err := os.Open(path) + raw, err := os.ReadFile(path) if err != nil { - return nil, fmt.Errorf("opening config file %q: %w", path, err) + return nil, fmt.Errorf("reading config file %q: %w", path, err) } - defer func() { _ = f.Close() }() var config Config - if err := toml.NewDecoder(f).Decode(&config); err != nil { + if err := toml.Unmarshal(raw, &config); err != nil { return nil, fmt.Errorf("decoding config file %q: %w", path, err) } + warnUnknownKeys(path, raw) + config.SetDefaults() if err := config.Validate(); err != nil { @@ -34,6 +36,25 @@ func LoadConfig(path string) (*Config, error) { return &config, nil } +// warnUnknownKeys reports keys the decode silently ignored. Rejecting them +// would break configurations that already carry a stray key, but dropping them +// without a word hides the mistypes ([[Recipient]], Adresses) that otherwise +// produce a server which accepts mail and forwards none of it. +func warnUnknownKeys(path string, raw []byte) { + var discard Config + + var strict *toml.StrictMissingError + if !errors.As(toml.NewDecoder(bytes.NewReader(raw)).DisallowUnknownFields().Decode(&discard), &strict) { + return + } + + for i := range strict.Errors { + slog.Warn("ignoring unknown key in config file", + slog.String("path", path), + slog.String("key", strings.Join(strict.Errors[i].Key(), "."))) + } +} + type Config struct { Port int Username string @@ -118,6 +139,13 @@ func (cr *ConfigRecipient) GetTargetURLs() []*url.URL { slog.String("err", err.Error())) continue // Skip invalid URLs } + // url.Parse accepts almost any string, so the scheme is what + // actually distinguishes a shoutrrr target from a typo. + if parsedURL.Scheme == "" { + slog.Error("shoutrrr target URL has no scheme", + slog.String("target", target)) + continue + } cr.targetURLs = append(cr.targetURLs, parsedURL) } } diff --git a/config_test.go b/config_test.go index 686f048..f5a1820 100644 --- a/config_test.go +++ b/config_test.go @@ -1,6 +1,8 @@ package smtp2shoutrrr import ( + "bytes" + "log/slog" "os" "path/filepath" "testing" @@ -99,6 +101,11 @@ Addresses = ["user@example.com"] [[Recipients]] Addresses = ["user@example.com"] Targets = ["://invalid-url"] +`, + "recipient whose target is not a URL at all": ` +[[Recipients]] +Addresses = ["user@example.com"] +Targets = ["this is not a url at all"] `, "catch-all without targets": ` [CatchAll] @@ -124,3 +131,26 @@ Targets = ["ntfy://ntfy.sh/catch-all"] require.Empty(t, config.Recipients) require.NotNil(t, config.CatchAll) } + +// A mistyped table name is silently dropped by the decoder. It is only fatal +// when nothing else is configured, so with a catch-all present the warning is +// the sole signal that the block was ignored. +func TestLoadConfigWarnsAboutIgnoredKeys(t *testing.T) { + var logged bytes.Buffer + restore := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logged, &slog.HandlerOptions{Level: slog.LevelWarn}))) + t.Cleanup(func() { slog.SetDefault(restore) }) + + config, err := LoadConfig(writeConfig(t, ` +[[Recipient]] +Addresses = ["user@example.com"] +Targets = ["ntfy://ntfy.sh/topic"] + +[CatchAll] +Targets = ["ntfy://ntfy.sh/catch-all"] +`)) + require.NoError(t, err) + require.Empty(t, config.Recipients, "the mistyped table is still ignored") + require.Contains(t, logged.String(), "ignoring unknown key in config file") + require.Contains(t, logged.String(), "Recipient") +} diff --git a/server.go b/server.go index 20aacb5..21b648b 100644 --- a/server.go +++ b/server.go @@ -16,16 +16,20 @@ const ( readTimeout = 10 * time.Second writeTimeout = 10 * time.Second - // shutdownTimeout bounds the drain of in-flight sessions. It must stay - // clear of readTimeout: an idle client is only dropped once its read - // deadline expires, so a budget at or below readTimeout would report an + // defaultShutdownTimeout bounds the drain of in-flight sessions. It must + // stay well clear of readTimeout: an idle client is only dropped once its + // read deadline expires, so a budget near readTimeout would report an // ordinary drain as a failed shutdown. - shutdownTimeout = readTimeout + 20*time.Second + defaultShutdownTimeout = readTimeout + 20*time.Second ) type Server struct { backend *smtp.Server + // shutdownTimeout is a field rather than the constant so tests can drive a + // real budget overrun without waiting out readTimeout. + shutdownTimeout time.Duration + mu sync.Mutex listener net.Listener addr string @@ -80,24 +84,49 @@ func (s *Server) Start(ctx context.Context) error { select { case err := <-served: + // Serve gave up on its own; still release the listener and drain any + // sessions it left running before reporting why. + s.closeListener() + s.drain(context.WithoutCancel(ctx)) return err case <-ctx.Done(): } - // ctx is already cancelled, so the drain budget needs a live context. - shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), shutdownTimeout) + // Stop accepting and let Serve return before draining. go-smtp registers + // each accepted session on a WaitGroup that Shutdown waits on from another + // goroutine, so an Accept still in flight would Add to that WaitGroup + // concurrently with the Wait — a data race, and a session the drain then + // fails to wait for. + s.closeListener() + if err := <-served; err != nil && !errors.Is(err, net.ErrClosed) { + return err + } + + s.drain(context.WithoutCancel(ctx)) + + return nil +} + +// drain stops the backend and waits for in-flight sessions to finish. Running +// past the budget is still an intentional stop, so it is logged rather than +// reported: turning it into an error exits the process non-zero on a routine +// signal. +func (s *Server) drain(ctx context.Context) { + shutdownCtx, cancel := context.WithTimeout(ctx, s.shutdownTimeout) defer cancel() - if err := s.Stop(shutdownCtx); err != nil && !errors.Is(err, smtp.ErrServerClosed) { - // Outrunning the drain budget is still an intentional stop; reporting - // it as a failure would exit the process non-zero on a routine signal. + // Shutdown also closes the listener, which we have already closed, so its + // close error is expected rather than a sign of a session still running. + if err := s.Stop(shutdownCtx); err != nil && + !errors.Is(err, smtp.ErrServerClosed) && !errors.Is(err, net.ErrClosed) { slog.Warn("SMTP server stopped with sessions still in flight", slog.String("err", err.Error())) } - - return <-served } +// Stop shuts the server down and releases its listener. A Server is single +// use: the backend cannot be restarted once stopped, so a later Start binds +// the address and then immediately gives it up. func (s *Server) Stop(ctx context.Context) error { slog.Info("Stopping SMTP server") @@ -121,6 +150,7 @@ func NewSMTPServer(config *Config) *Server { smtpBackend.AllowInsecureAuth = true return &Server{ - backend: smtpBackend, + backend: smtpBackend, + shutdownTimeout: defaultShutdownTimeout, } } diff --git a/server_test.go b/server_test.go index 1dadc8d..bebf864 100644 --- a/server_test.go +++ b/server_test.go @@ -34,8 +34,11 @@ func canDial(addr string) bool { return conn.Close() == nil } -// startServer starts srv on an ephemeral port and blocks until it accepts -// connections, returning the dial address and the channel Start returns on. +// startServer starts srv on an ephemeral port and blocks until it is bound, +// returning the dial address and the channel Start returns on. Readiness is +// the bind rather than a probe connection: the address is set before Serve +// runs, connections queue in the backlog either way, and a probe that opens +// and drops a connection only adds accept churn and server-side log noise. func startServer(t *testing.T, srv *Server) (string, context.CancelFunc, <-chan error) { t.Helper() @@ -48,8 +51,16 @@ func startServer(t *testing.T, srv *Server) (string, context.CancelFunc, <-chan }() require.Eventually(t, func() bool { - return canDial(loopbackAddr(srv)) - }, 5*time.Second, 20*time.Millisecond, "server never started listening") + return srv.Addr() != "" || len(stopped) > 0 + }, 5*time.Second, 10*time.Millisecond, "server never started listening") + + // Report why Start gave up rather than letting it look like a slow bind. + select { + case err := <-stopped: + require.NoError(t, err, "server failed to start") + t.Fatal("server stopped before it began serving") + default: + } return loopbackAddr(srv), cancel, stopped } @@ -67,7 +78,7 @@ func TestServerShutsDownOnContextCancel(t *testing.T) { select { case err := <-stopped: require.NoError(t, err) - case <-time.After(shutdownTimeout + time.Second): + case <-time.After(defaultShutdownTimeout + time.Second): t.Fatal("server did not shut down after context cancellation") } @@ -134,15 +145,54 @@ func TestServerDrainsIdleConnectionWithoutError(t *testing.T) { select { case err := <-stopped: require.NoError(t, err, "an intentional shutdown must not report an error") - case <-time.After(shutdownTimeout + time.Second): + case <-time.After(defaultShutdownTimeout + time.Second): t.Fatal("server did not drain the idle connection") } } -// The drain waits out a client's read deadline, so the budget has to exceed it -// or every stop with an idle connection reports a failure. -func TestShutdownBudgetExceedsReadTimeout(t *testing.T) { - require.Greater(t, shutdownTimeout, readTimeout) +// The drain waits out a client's read deadline, so the budget needs real room +// above it. A strict inequality is not enough: a budget one millisecond over +// readTimeout is the same coin flip as one equal to it. +func TestShutdownBudgetLeavesMarginOverReadTimeout(t *testing.T) { + require.GreaterOrEqual(t, defaultShutdownTimeout, readTimeout+10*time.Second) +} + +// Regression: overrunning the drain budget is still an intentional stop. +// Returning that deadline as an error made the command exit non-zero on a +// routine SIGTERM. Driven with a real overrun — a client that stays connected +// past a deliberately short budget — rather than asserted on the constant. +func TestServerReportsNoErrorWhenDrainOverrunsBudget(t *testing.T) { + srv := NewSMTPServer(testConfig()) + // Long enough that the session outlives the budget, short enough that it + // does not outlive the test binary. + srv.backend.ReadTimeout = 3 * time.Second + srv.shutdownTimeout = 300 * time.Millisecond + + addr, cancel, stopped := startServer(t, srv) + + conn, err := net.DialTimeout("tcp", addr, time.Second) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + + // Wait for the greeting so the session is registered and genuinely in + // flight when the drain starts. + _ = conn.SetReadDeadline(time.Now().Add(2 * time.Second)) + buf := make([]byte, 64) + _, err = conn.Read(buf) + require.NoError(t, err) + + start := time.Now() + cancel() + + select { + case err := <-stopped: + require.NoError(t, err, "a drain that outruns its budget is still an intentional stop") + case <-time.After(srv.backend.ReadTimeout + 5*time.Second): + t.Fatal("Start did not return after the drain budget expired") + } + + require.Less(t, time.Since(start), srv.backend.ReadTimeout, + "Start should return on the budget, not wait out the read deadline") } func mustAtoi(t *testing.T, s string) int { -- 2.52.0