Skip to content

fix(mailer): time out when the SMTP server never sends its greeting - #2043

Open
RyoKusnadi wants to merge 1 commit into
raystack:mainfrom
RyoKusnadi:fix/mailer-greeting-deadline
Open

RyoKusnadi wants to merge 1 commit into
raystack:mainfrom
RyoKusnadi:fix/mailer-greeting-deadline

Conversation

@RyoKusnadi

Copy link
Copy Markdown

Summary

An email send hangs forever if the SMTP server accepts the connection but never sends its greeting. This sets a deadline as soon as the connection opens, so the send fails after the dialer timeout (10s by default).

How it happens: a mail server (or a proxy or firewall in front of it) accepts the TCP connection and then stays silent. gopkg.in/mail.v2 Dialer.Dial reads the greeting (smtp.go:91, inside smtpNewClient) before it sets the connection deadline (smtp.go:96-98), so that read has no deadline. Every mail path goes through DialerImpl.DialAndSend: login OTP, magic link, invitations, PAT alerts, org and user delete emails. Most run inside a request, so the request hangs too. It needs a silent server, so it is rare.

Changes

  • pkg/mailer/dialer.go: wrap mail.NetDialTimeout once (sync.Once, from NewDialerImpl) so each new connection gets a deadline of the dialer Timeout right away. The library then resets the deadline after the greeting as before.
  • pkg/mailer/dialer_test.go: new test.

Technical Details

Cause: the library sets the deadline after reading the greeting, not before.
Why this way: the issue lists two options. A wrapper on the library's own NetDialTimeout hook is a few lines. Moving to another SMTP library is a much larger change.
Scope: NetDialTimeout is a package-level variable, so the wrapper applies to every mail.v2 dialer in the process. pkg/mailer is the only place in the repo that creates one (grep NewDialer). The wrapper only adds a deadline of the dialer's own timeout, and does nothing if the timeout is 0.

Test Plan

  • Build and type checking passes
  • Manual testing completed (not done against a real mail server)

Test TestDialerImpl_DialAndSend_SilentServer starts a local TCP listener on 127.0.0.1 that accepts and never writes.

  • Before the fix: FAIL ... DialAndSend still blocked after 3s: no deadline on the greeting read
  • After the fix: ok github.com/raystack/frontier/pkg/mailer 0.540s
  • Commands: go vet ./pkg/mailer/..., go test ./pkg/mailer/..., gofmt -l pkg/mailer. The full suite and integration tests were not run.

Risk: low. A server that is slow but not silent now has the dialer timeout to send its greeting, which is the timeout already meant for the whole connection setup.

Fixes #2039

Signed-off-by: Ryo Kusnadi <ryokusnadi@gmail.com>
@vercel

vercel Bot commented Nov 3, 2026

Copy link
Copy Markdown

@RyoKusnadi is attempting to deploy a commit to the Raystack Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Summary

Summary by CodeRabbit

  • Bug Fixes
    • Mail delivery now respects the configured timeout when connecting to an unresponsive mail server, preventing sends from remaining blocked indefinitely.
    • Connection setup failures are returned promptly, so mail operations can report errors instead of waiting without a response.

The mail dialer now sets a deadline immediately after a successful connection when the configured timeout is positive. A test checks that sending to a server that accepts connections but sends no greeting returns an error.

Changes: SMTP greeting timeout

Layer / File(s) Summary
Install and verify greeting deadline
pkg/mailer/dialer.go, pkg/mailer/dialer_test.go
NewDialerImpl installs a wrapper around mail.NetDialTimeout. The wrapper sets a deadline on successful connections when the timeout is positive and closes the connection if setting the deadline fails. The test checks that DialAndSend returns an error when the server does not send a greeting.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

  • raystack/frontier#2034: Both changes use the shared mail send path. This PR makes a send to a silent SMTP server return an error rather than block.

Suggested reviewers: rohilsurana

Merge Risk: Low · up to 75d05

The change prevents indefinite waits for silent SMTP servers. It is mergeable with a bounded test improvement: verify that failure follows the configured timeout rather than merely occurring within three seconds.

Pre-merge checks: Passed 2
Check name Status Explanation
Linked Issues check Passed [#2039] The only direct linked issue requires that the SMTP greeting read not block indefinitely when the server accepts a connection and stays silent. At head, pkg/mailer/dialer.go adds `installGre…
Out of Scope Changes check Passed The whole-PR diff against the merge base eaa557a5 changes two files. pkg/mailer/dialer.go adds the one-time NetDialTimeout wrapper and calls it from NewDialerImpl. pkg/mailer/dialer_test.go …
Full details: Linked Issues check

Explanation

[#2039] The only direct linked issue requires that the SMTP greeting read not block indefinitely when the server accepts a connection and stays silent. At head, pkg/mailer/dialer.go adds installGreetingDeadline(), which wraps mail.NetDialTimeout once through sync.Once. NewDialerImpl calls it before mail.NewDialer. The wrapper calls the original dial, then sets conn.SetDeadline(time.Now().Add(timeout)) when timeout &gt; 0. The greeting read therefore has a finite deadline before the library reads the greeting. This matches the fix proposed in the issue. pkg/mailer/dialer_test.go adds TestDialerImpl_DialAndSend_SilentServer. It uses a local listener that accepts and never writes, and it expects DialAndSend to return an error within 3s with a 200 ms dialer timeout. Limit of evidence: I could not read the gopkg.in/mail.v2 source. The repository has no vendor copy, so I could not confirm that the library passes d.Timeout as the timeout argument of the hook. The author reports the test failing before the change and passing after it (0.540s). I did not rerun the test. The requirement is met in the default configuration, where the wrapper bounds the greeting read to the library's finite dial timeout.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff against the merge base eaa557a5 changes two files. pkg/mailer/dialer.go adds the one-time NetDialTimeout wrapper and calls it from NewDialerImpl. pkg/mailer/dialer_test.go is a new test for the silent-server case. Both changes implement the objective of #2039. The wrapper is process-wide because NetDialTimeout is a package variable. The PR description discloses this. The repository creates mail.v2 dialers only in pkg/mailer, so no unrelated dialer is affected. No unrelated files or behavior changes appear in the reviewed diff.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Follow-up Comments

Nitpick Assert the greeting timeout in the silent-server test.

44-46: Trivial · Maintainability & Code Quality · Quick win

Assert the greeting timeout in the silent-server test.

The test allows up to three seconds and accepts any returned error. A greeting deadline near two seconds would still pass, even though the test comment says the send fails after the dialer timeout of 200 ms. Record the start time before the send. After the error, assert that the elapsed time stays under one second. This leaves margin for scheduling delay above the 200 ms timeout.

Suggested fix
 	done := make(chan error, 1)
+	start := time.Now()
 	go func() { done <- d.DialAndSend(msg) }()
 
 	select {
 	case err := <-done:
 		assert.Error(t, err)
+		assert.Less(t, time.Since(start), time.Second, "greeting read must end near the 200ms dialer timeout")

Open the source location

Review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between eaa557a and 75d05a4.

Files selected for processing: 2
  • pkg/mailer/dialer.go
  • pkg/mailer/dialer_test.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An SMTP server that never sends its greeting makes an email send hang

1 participant