You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(mailer): time out when the SMTP server never sends its greeting - #2043
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.v2Dialer.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, and checks that the send fails within one second.
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.
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Bug Fixes
Mail delivery now applies the configured timeout to connections with an unresponsive mail server, preventing sends from remaining blocked indefinitely.
Connection setup failures, including errors while applying the timeout, are returned promptly so mail operations can report the problem rather than 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.
raystack/frontier#2034: This PR sends a user-deletion notice through the shared mail dialer. This change sets a deadline on that shared path before the SMTP greeting is read.
Suggested reviewers:rohilsurana
Merge Risk: Minimal · up to 02773
The SMTP greeting now has a deadline, and the relevant timeout behavior is covered by the test.
Pre-merge checks: 2
Check name
Status
Explanation
Linked Issues check
[#2039] The only directly linked issue requires that the SMTP greeting read not block indefinitely when the server accepts a connection and stays silent. The whole-PR diff against merge base eaa557a …
Out of Scope Changes check
The whole-PR diff against merge base eaa557a changes two files only. pkg/mailer/dialer.go adds the one-time NetDialTimeout wrapper and its call in NewDialerImpl. pkg/mailer/dialer_test.go is …
Full details: Linked Issues check
Explanation
[#2039] The only directly linked issue requires that the SMTP greeting read not block indefinitely when the server accepts a connection and stays silent. The whole-PR diff against merge base eaa557a shows installGreetingDeadline() in pkg/mailer/dialer.go. It wraps mail.NetDialTimeout once through sync.Once. NewDialerImpl calls it before mail.NewDialer. The wrapper calls the original dial. When timeout > 0, it calls conn.SetDeadline(time.Now().Add(timeout)) before the library reads the greeting. This matches the first fix option in the issue. pkg/mailer/dialer_test.go adds TestDialerImpl_DialAndSend_SilentServer. The test uses a local listener that accepts and never writes. It expects DialAndSend to return an error within 3s, with a 200 ms dialer timeout. Evidence limit: the gopkg.in/mail.v2 source is not in the repository, so I could not confirm that Dial routes through the NetDialTimeout hook or which timeout value it passes. The author reports the test failing before the change and passing after it (0.540s). I did not rerun the test. Even if the library passes a fixed timeout instead of d.Timeout, the wrapper still gives the greeting read a finite deadline. The requirement is met in both cases.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff against merge base eaa557a changes two files only. pkg/mailer/dialer.go adds the one-time NetDialTimeout wrapper and its call in 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 PR description also states that pkg/mailer is the only place in the repository that creates a mail.v2 dialer. The change adds no unrelated files or behavior. The NewDialerImpl call and the test both use pkg/mailer code only.
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.
Reviewing files that changed from the base of the PR and between 75d05a4 and 027737c.
Files selected for processing: 1
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.
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")
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.
Good point, the test now also checks that the send fails within a second of starting, so a slow timeout can't pass it (027737c). Updated the description too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.v2Dialer.Dialreads the greeting (smtp.go:91, insidesmtpNewClient) before it sets the connection deadline (smtp.go:96-98), so that read has no deadline. Every mail path goes throughDialerImpl.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: wrapmail.NetDialTimeoutonce (sync.Once, fromNewDialerImpl) so each new connection gets a deadline of the dialerTimeoutright 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
NetDialTimeouthook is a few lines. Moving to another SMTP library is a much larger change.Scope:
NetDialTimeoutis a package-level variable, so the wrapper applies to everymail.v2dialer in the process.pkg/maileris 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
Test
TestDialerImpl_DialAndSend_SilentServerstarts a local TCP listener on 127.0.0.1 that accepts and never writes, and checks that the send fails within one second.FAIL ... DialAndSend still blocked after 3s: no deadline on the greeting readok github.com/raystack/frontier/pkg/mailer 0.540sgo 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