Skip to content

fix(email): add SMTP client timeouts to prevent indefinite hanging #51

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ilramdhan merged 2 commits into from
Apr 14, 2026
+27 −2
Merged

fix(email): add SMTP client timeouts to prevent indefinite hanging #51

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
331f073
fix(email): add SMTP client timeouts to prevent indefinite hanging
ilramdhan Apr 14, 2026
9e1b2c3
fix(email): handle errors when closing SMTP connection after SetDeadl…
ilramdhan Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions services/iam/internal/infrastructure/email/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,22 @@ import (
"context"
"crypto/tls"
"fmt"
"net"
"net/smtp"
"strings"
"time"

"github.com/rs/zerolog/log"

"github.com/mutugading/goapps-backend/services/iam/internal/infrastructure/config"
)

// SMTP client timeouts. Kept tight so failures surface fast and never block the request path.
const (
smtpDialTimeout = 10 * time.Second
smtpOverallTimeout = 30 * time.Second
)
Comment on lines +18 to +22
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment claims the SMTP timeouts "never block the request path", but the code can still block the caller for up to smtpOverallTimeout (and up to smtpDialTimeout during connect). Consider rewording to reflect that the goal is to bound/limit blocking time rather than eliminate it entirely.

Copilot uses AI. Check for mistakes.

// Service implements the auth.EmailService interface via SMTP.
type Service struct {
cfg *config.EmailConfig
Expand Down Expand Up @@ -70,7 +78,7 @@ func (s *Service) send(ctx context.Context, to, subject, htmlBody string) error

var msg strings.Builder
for k, v := range headers {
fmt.Fprintf(&msg, "%s: %s\r\n", k, v)
_, _ = fmt.Fprintf(&msg, "%s: %s\r\n", k, v)
}
msg.WriteString("\r\n")
msg.WriteString(htmlBody)
Expand All @@ -96,6 +104,10 @@ func (s *Service) send(ctx context.Context, to, subject, htmlBody string) error
}

func (s *Service) sendTLS(ctx context.Context, addr string, auth smtp.Auth, to, msg string) error {
// Enforce a bounded overall deadline so no SMTP step can hang indefinitely.
ctx, cancel := context.WithTimeout(ctx, smtpOverallTimeout)
defer cancel()

Comment on lines 106 to +110
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timeouts are only enforced in sendTLS; the non-TLS path still uses smtp.SendMail (which dials without timeouts) and can still hang indefinitely. If the intent is truly to ensure email sending never hangs, consider adding the same dial/deadline approach to the non-TLS path (custom net.Dialer + conn.SetDeadline + smtp.NewClient).

Copilot uses AI. Check for mistakes.
tlsConfig := &tls.Config{
ServerName: s.cfg.SMTPHost,
MinVersion: tls.VersionTLS12,
Expand All @@ -104,12 +116,25 @@ func (s *Service) sendTLS(ctx context.Context, addr string, auth smtp.Auth, to,
tlsConfig.InsecureSkipVerify = true //nolint:gosec // Configurable for dev/self-hosted environments.
}

dialer := &tls.Dialer{Config: tlsConfig}
dialer := &tls.Dialer{
NetDialer: &net.Dialer{Timeout: smtpDialTimeout},
Config: tlsConfig,
}
conn, err := dialer.DialContext(ctx, "tcp", addr)
if err != nil {
return fmt.Errorf("failed to connect to SMTP server: %w", err)
}

// After dial, TCP-level deadlines prevent a stalled server from hanging reads/writes.
if deadline, ok := ctx.Deadline(); ok {
if err := conn.SetDeadline(deadline); err != nil {
if closeErr := conn.Close(); closeErr != nil {
log.Warn().Err(closeErr).Msg("failed to close SMTP connection after SetDeadline error")
}
return fmt.Errorf("failed to set SMTP connection deadline: %w", err)
}
}

client, err := smtp.NewClient(conn, s.cfg.SMTPHost)
if err != nil {
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If smtp.NewClient returns an error, the underlying conn is not closed, which can leak file descriptors on handshake/banner failures. Close conn on this error path (or defer conn.Close immediately after DialContext and cancel the defer when the client takes ownership).

Suggested change
if err != nil {
if err != nil {
_ = conn.Close()

Copilot uses AI. Check for mistakes.
return fmt.Errorf("failed to create SMTP client: %w", err)
Expand Down
Loading