Retry transient (4xx) SMTP replies, including on the DATA terminator

pool.go

@@ -305,8 +305,11 @@ func (p *Pool) returnConn(c *conn, lastErr error) (err error) {
 		// All non-textproto errors should close the connection.
 		if err, ok := lastErr.(*textproto.Error); !ok {
 			return lastErr
-		} else if err.Code == 421 {
-			// As an exception, 421 (rate-limit) errors should also close the connection.
+		} else if err.Code >= 400 && err.Code < 500 {
+			// As an exception, transient (4xx) replies (eg: 421 rate-limit,
+			// 451 timeout) should also close the connection: the session may
+			// be in a compromised state, and any retry must dial a fresh
+			// connection rather than reuse this one.
 			return lastErr
 		}
 	}
@@ -442,6 +445,16 @@ func (c *conn) send(e Email) (bool, error) {
 	}
 
 	if err := w.Close(); err != nil {
+		// w.Close() writes the terminating "." and reads the server's final
+		// reply. A transient (4xx) reply here means the server explicitly did
+		// not accept the message (eg: SES "451 Timeout waiting for data from
+		// client." on a stale pooled connection), so it is safe to retry on a
+		// fresh connection. A connection-level error is deliberately NOT
+		// retried here: the server may have accepted the message before the
+		// socket dropped, and retrying could cause duplicate delivery.
+		if tperr, ok := err.(*textproto.Error); ok && tperr.Code >= 400 && tperr.Code < 500 {
+			return true, err
+		}
 		return false, err
 	}
 	isClosed = true
@@ -493,16 +506,24 @@ func combineEmails(lists ...[]string) ([]string, error) {
 	return out, nil
 }
 
-// canRetry returns true if the given SMTP err is network
-// related and hence, can be retried.
-// eg: TCP/DNS/timeout/broken pipe etc.
+// canRetry returns true if the given SMTP err is network related or a
+// transient (4xx) SMTP reply, and hence, can be retried.
+// eg: TCP/DNS/timeout/broken pipe, or "451 Timeout waiting for data" etc.
 func canRetry(err error) bool {
 	if errors.As(err, &netErr) {
 		return true
 	} else if _, ok := err.(*net.OpError); ok {
 		return true
 	} else if err == io.EOF {
 		return true
+	} else if tperr, ok := err.(*textproto.Error); ok {
+		// Transient (4xx) SMTP replies are safe to retry on a fresh
+		// connection: the server explicitly did not accept the message, so a
+		// retry cannot cause duplicate delivery. Permanent (5xx) replies
+		// (bad recipient, message rejected, etc.) must not be retried.
+		// eg: AWS SES returns "451 4.4.2 Timeout waiting for data from
+		// client." when a pooled connection has gone stale.
+		return tperr.Code >= 400 && tperr.Code < 500
 	}
 
 	return false

retry_test.go

@@ -0,0 +1,45 @@
+package smtppool
+
+import (
+	"errors"
+	"io"
+	"net"
+	"net/textproto"
+	"testing"
+)
+
+// TestCanRetry verifies the retriability classification: transient (4xx) SMTP
+// replies and connection-level errors are retriable; permanent (5xx) replies
+// and non-transient errors are not.
+func TestCanRetry(t *testing.T) {
+	cases := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		// Transient SMTP replies (4xx) are safe to retry.
+		{"ses 451 data timeout", &textproto.Error{Code: 451, Msg: "4.4.2 Timeout waiting for data from client."}, true},
+		{"421 rate limit", &textproto.Error{Code: 421, Msg: "Too many connections"}, true},
+		{"450 mailbox busy", &textproto.Error{Code: 450, Msg: "Requested mail action not taken"}, true},
+
+		// Permanent SMTP replies (5xx) must NOT be retried.
+		{"550 no such user", &textproto.Error{Code: 550, Msg: "No such user"}, false},
+		{"552 message too large", &textproto.Error{Code: 552, Msg: "Message size exceeds limit"}, false},
+
+		// Connection-level errors are retriable.
+		{"io.EOF", io.EOF, true},
+		{"net.OpError", &net.OpError{Op: "dial", Err: errors.New("connection refused")}, true},
+
+		// Everything else is not retriable.
+		{"plain error", errors.New("some non-network error"), false},
+		{"nil", nil, false},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := canRetry(tc.err); got != tc.want {
+				t.Errorf("canRetry(%v) = %v, want %v", tc.err, got, tc.want)
+			}
+		})
+	}
+}