From 7dcad0d1c1dcb30477d72aaaacc7bbafccc797da Mon Sep 17 00:00:00 2001 From: josephsellers <6892567+josephsellers@users.noreply.github.com> Date: Wed, 15 Jul 2026 14:41:24 +0100 Subject: [PATCH] Retry transient (4xx) SMTP replies, including on the DATA terminator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit canRetry() only classified connection-level errors (net.Error, net.OpError, io.EOF) as retriable, and conn.send() hardcoded `return false` for the error from w.Close() -- the reply to the DATA terminator ("."). As a result, a transient 4xx reply at end-of-DATA was never retried even with MaxMessageRetries set, and surfaced to the caller as a hard failure. This is common with AWS SES, which returns "451 4.4.2 Timeout waiting for data from client." on a pooled connection whose server-side state has gone stale; an immediate retry on a fresh connection succeeds. - canRetry(): also treat a 4xx *textproto.Error as retriable (5xx stays permanent, so bad-recipient etc. is not retried). - send()/w.Close(): retry on a 4xx reply, but NOT on a connection-level error there -- after the terminator the server may have accepted the message before the socket dropped, so retrying could duplicate delivery. - returnConn(): generalise the 421-closes-connection rule (#19) to all 4xx, so a retry dials a fresh connection rather than reusing the poisoned one. Adds TestCanRetry covering 4xx/5xx/connection/other classification. [Used Claude Code 🤖] --- pool.go | 31 ++++++++++++++++++++++++++----- retry_test.go | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 5 deletions(-) create mode 100644 retry_test.go diff --git a/pool.go b/pool.go index 552f174..ff89a37 100644 --- a/pool.go +++ b/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,9 +506,9 @@ 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 @@ -503,6 +516,14 @@ func canRetry(err error) bool { 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 diff --git a/retry_test.go b/retry_test.go new file mode 100644 index 0000000..2973fbf --- /dev/null +++ b/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) + } + }) + } +}