From d660f88f678b6520a36f42ac15270f902d43aad2 Mon Sep 17 00:00:00 2001 From: Alberto Bertogli Date: Mon, 17 Oct 2016 20:59:57 +0100 Subject: [PATCH] queue: Send DSN for messages that time out in the queue The queue currently only considers failed recipients when deciding whether to send a DSN or not. This is a bug, as recipients that time out are not taken into account. This patch fixes that issue by including both failed and pending recipients in the DSN. It also adds more comprehensive tests for this case, both in the queue and in the dsn generation code. --- internal/queue/dsn.go | 44 +++++++---- internal/queue/dsn_test.go | 114 ++++++++++++++++++++++++++-- internal/queue/queue.go | 11 ++- internal/queue/queue_test.go | 40 ++++++++++ test/t-05-null_address/expected_dsr | 4 +- 5 files changed, 186 insertions(+), 27 deletions(-) diff --git a/internal/queue/dsn.go b/internal/queue/dsn.go index 4475caf..7ee2243 100644 --- a/internal/queue/dsn.go +++ b/internal/queue/dsn.go @@ -23,11 +23,18 @@ func deliveryStatusNotification(domainFrom string, item *Item) ([]byte, error) { Date: time.Now().Format(time.RFC1123Z), To: item.To, Recipients: item.Rcpt, + FailedTo: map[string]string{}, } for _, rcpt := range item.Rcpt { - if rcpt.Status == Recipient_FAILED { - info.FailedRcpts = append(info.FailedRcpts, rcpt) + if rcpt.Status != Recipient_SENT { + info.FailedTo[rcpt.OriginalAddress] = rcpt.OriginalAddress + switch rcpt.Status { + case Recipient_FAILED: + info.FailedRecipients = append(info.FailedRecipients, rcpt) + case Recipient_PENDING: + info.PendingRecipients = append(info.PendingRecipients, rcpt) + } } } @@ -43,14 +50,16 @@ func deliveryStatusNotification(domainFrom string, item *Item) ([]byte, error) { } type dsnInfo struct { - OurDomain string - Destination string - MessageID string - Date string - To []string - Recipients []*Recipient - FailedRcpts []*Recipient - OriginalMessage string + OurDomain string + Destination string + MessageID string + Date string + To []string + FailedTo map[string]string + Recipients []*Recipient + FailedRecipients []*Recipient + PendingRecipients []*Recipient + OriginalMessage string } var dsnTemplate = template.Must(template.New("dsn").Parse( @@ -59,17 +68,22 @@ To: <{{.Destination}}> Subject: Mail delivery failed: returning message to sender Message-ID: <{{.MessageID}}> Date: {{.Date}} -X-Failed-Recipients: {{range .To}}{{.}}, {{end}} +X-Failed-Recipients: {{range .FailedTo}}{{.}}, {{end}} Auto-Submitted: auto-replied Delivery to the following recipient(s) failed permanently: - {{range .To -}} - {{.}} - {{end}} + {{range .FailedTo -}} - {{.}} + {{- end}} + ----- Technical details ----- -{{range .Recipients}} -- "{{.Address}}" ({{.Type}}) failed with error: +{{range .FailedRecipients}} +- "{{.Address}}" ({{.Type}}) failed permanently with error: + {{.LastFailureMessage}} +{{end}} +{{- range .PendingRecipients}} +- "{{.Address}}" ({{.Type}}) failed repeatedly and timed out, last error: {{.LastFailureMessage}} {{end}} diff --git a/internal/queue/dsn_test.go b/internal/queue/dsn_test.go index 2068d08..d4429fd 100644 --- a/internal/queue/dsn_test.go +++ b/internal/queue/dsn_test.go @@ -1,6 +1,11 @@ package queue -import "testing" +import ( + "fmt" + "sort" + "strings" + "testing" +) func TestDSN(t *testing.T) { item := &Item{ @@ -10,9 +15,12 @@ func TestDSN(t *testing.T) { To: []string{"toto@africa.org", "negra@sosa.org"}, Rcpt: []*Recipient{ {"poe@rcpt", Recipient_EMAIL, Recipient_FAILED, - "oh! horror!", ""}, - {"newman@rcpt", Recipient_EMAIL, Recipient_FAILED, - "oh! the humanity!", ""}}, + "oh! horror!", "toto@africa.org"}, + {"newman@rcpt", Recipient_EMAIL, Recipient_PENDING, + "oh! the humanity!", "toto@africa.org"}, + {"ant@rcpt", Recipient_EMAIL, Recipient_SENT, + "", "negra@sosa.org"}, + }, Data: []byte("data ñaca"), }, } @@ -21,6 +29,100 @@ func TestDSN(t *testing.T) { if err != nil { t.Error(err) } - - t.Log(string(msg)) + if !flexibleEq(expectedDSN, string(msg)) { + t.Errorf("generated DSN different than expected") + printDiff(func(s string) { t.Error(s) }, expectedDSN, string(msg)) + } else { + t.Log(string(msg)) + } +} + +const expectedDSN = `From: Mail Delivery System +To: +Subject: Mail delivery failed: returning message to sender +Message-ID: +Date: * +X-Failed-Recipients: toto@africa.org, +Auto-Submitted: auto-replied + +Delivery to the following recipient(s) failed permanently: + + - toto@africa.org + + +----- Technical details ----- + +- "poe@rcpt" (EMAIL) failed permanently with error: + oh! horror! + +- "newman@rcpt" (EMAIL) failed repeatedly and timed out, last error: + oh! the humanity! + + +----- Original message ----- + +data ñaca + +` + +// flexibleEq compares two strings, supporting wildcards. +// Not particularly nice or robust, only useful for testing. +func flexibleEq(expected, got string) bool { + posG := 0 + for i := 0; i < len(expected); i++ { + if posG >= len(got) { + return false + } + + c := expected[i] + if c == '?' { + posG++ + continue + } else if c == '*' { + for got[posG] != '\n' { + posG++ + } + continue + } else if byte(c) != got[posG] { + return false + } + + posG++ + } + + return true +} + +// printDiff prints the difference between the strings using the given +// function. This is a _horrible_ implementation, only useful for testing. +func printDiff(print func(s string), expected, got string) { + lines := []string{} + + // expected lines and map. + eM := map[string]int{} + for _, l := range strings.Split(expected, "\n") { + eM[l]++ + lines = append(lines, l) + } + + // got lines and map. + gM := map[string]int{} + for _, l := range strings.Split(got, "\n") { + gM[l]++ + lines = append(lines, l) + } + + // sort the lines, to make it easier to see the differences (this works + // ok when there's few, horrible when there's lots). + sort.Strings(lines) + + // print diff of expected vs. got + seen := map[string]bool{} + print("E G | Line") + for _, l := range lines { + if !seen[l] && eM[l] != gM[l] { + print(fmt.Sprintf("%2d %2d | %q", eM[l], gM[l], l)) + seen[l] = true + } + } } diff --git a/internal/queue/queue.go b/internal/queue/queue.go index d598d93..7338933 100644 --- a/internal/queue/queue.go +++ b/internal/queue/queue.go @@ -334,7 +334,7 @@ func (item *Item) SendLoop(q *Queue) { } // Completed to all recipients (some may not have succeeded). - if item.countRcpt(Recipient_FAILED) > 0 && item.From != "<>" { + if item.countRcpt(Recipient_FAILED, Recipient_PENDING) > 0 && item.From != "<>" { sendDSN(tr, q, item) } @@ -420,11 +420,14 @@ func (item *Item) deliver(q *Queue, rcpt *Recipient) (err error, permanent bool) } // countRcpt counts how many recipients are in the given status. -func (item *Item) countRcpt(status Recipient_Status) int { +func (item *Item) countRcpt(statuses ...Recipient_Status) int { c := 0 for _, rcpt := range item.Rcpt { - if rcpt.Status == status { - c++ + for _, status := range statuses { + if rcpt.Status == status { + c++ + break + } } } return c diff --git a/internal/queue/queue_test.go b/internal/queue/queue_test.go index 649c7ef..3c58a91 100644 --- a/internal/queue/queue_test.go +++ b/internal/queue/queue_test.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "reflect" + "strings" "sync" "testing" "time" @@ -113,6 +114,45 @@ func TestBasic(t *testing.T) { } } +func TestDSNOnTimeout(t *testing.T) { + localC := newTestCourier() + remoteC := newTestCourier() + q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver(), + localC, remoteC, "dsndomain") + + // Insert an expired item in the queue. + item := &Item{ + Message: Message{ + ID: <-newID, + From: fmt.Sprintf("from@loco"), + Rcpt: []*Recipient{ + {"to@to", Recipient_EMAIL, Recipient_PENDING, "err", "to@to"}}, + Data: []byte("data"), + }, + CreatedAt: time.Now().Add(-24 * time.Hour), + } + q.q[item.ID] = item + err := item.WriteTo(q.path) + if err != nil { + t.Errorf("failed to write item: %v", err) + } + + // Launch the sending loop, expect 1 local delivery (the DSN). + localC.wg.Add(1) + go item.SendLoop(q) + localC.wg.Wait() + + req := localC.reqFor["from@loco"] + if req == nil { + t.Fatal("missing DSN") + } + + if req.from != "<>" || req.to != "from@loco" || + !strings.Contains(string(req.data), "X-Failed-Recipients: to@to,") { + t.Errorf("wrong DSN: %q", string(req.data)) + } +} + func TestFullQueue(t *testing.T) { q := New("/tmp/queue_test", set.NewString(), aliases.NewResolver(), dumbCourier, dumbCourier, "dsndomain") diff --git a/test/t-05-null_address/expected_dsr b/test/t-05-null_address/expected_dsr index 2f2a1c2..64b198c 100644 --- a/test/t-05-null_address/expected_dsr +++ b/test/t-05-null_address/expected_dsr @@ -10,11 +10,11 @@ Auto-Submitted: auto-replied Delivery to the following recipient(s) failed permanently: - fail@testserver - + ----- Technical details ----- -- "false" (PIPE) failed with error: +- "false" (PIPE) failed permanently with error: exit status 1