diff --git a/internal/app/worker/wmail/sync_imap.go b/internal/app/worker/wmail/sync_imap.go index 5ea0d00b..455cba9e 100644 --- a/internal/app/worker/wmail/sync_imap.go +++ b/internal/app/worker/wmail/sync_imap.go @@ -326,18 +326,33 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill return all, nil } +// threadParentID is the message this one answers, and the key its thread is +// built on. Only In-Reply-To carries that. +// +// Reply-To must not be used here. It is an address header -- "send replies to +// this mailbox" -- not a message identifier, so keying a thread on it puts +// every message a sender ever sent into one strand. On a production instance +// that collapsed 484 of 1431 stored messages into 62 threads: 76 unrelated +// DMARC aggregate reports from one reporter arrived as a single 76-message +// conversation, and a mailbox's own test sends and live outreach merged +// together. +// +// A message that answers nothing has no parent, and the caller roots its +// thread on its own Message-ID. +func threadParentID(msg *models.EmailMessageData) string { + if msg == nil || len(msg.InReplyTo) == 0 { + return "" + } + return msg.InReplyTo[len(msg.InReplyTo)-1] +} + // imapStore threads a new message and hands it to storeNew. func (w *WMail) imapStore(ctx context.Context, msg *models.EmailMessageData) error { msg.ID = uuid.New() now := time.Now() var threadID string - var parentID string - if len(msg.InReplyTo) > 0 { - parentID = msg.InReplyTo[len(msg.InReplyTo)-1] - } else if len(msg.ReplyTo) > 0 { - parentID = msg.ReplyTo[len(msg.ReplyTo)-1] - } + parentID := threadParentID(msg) if parentID != "" { internalParent, _ := w.EmailMessageMapRepository.Get(ctx, w.UserID, w.ID, parentID) diff --git a/internal/app/worker/wmail/thread_parent_test.go b/internal/app/worker/wmail/thread_parent_test.go new file mode 100644 index 00000000..126e2c9d --- /dev/null +++ b/internal/app/worker/wmail/thread_parent_test.go @@ -0,0 +1,80 @@ +package wmail + +import ( + "testing" + + "github.com/warmbly/warmbly/internal/models" +) + +// threadParentID decides which conversation an inbound message joins. A wrong +// answer does not lose the message, it files it under the wrong conversation, +// which is harder to notice and worse to work in. +// +// The case this pins: Reply-To as a fallback parent. It is an address header, +// not a message identifier, so keying on it puts every message a sender ever +// sent into one thread. +func TestThreadParentIDIgnoresReplyTo(t *testing.T) { + cases := []struct { + name string + msg *models.EmailMessageData + want string + }{ + { + name: "a genuine reply threads on In-Reply-To", + msg: &models.EmailMessageData{InReplyTo: []string{""}}, + want: "", + }, + { + name: "the last In-Reply-To wins on a deep chain", + msg: &models.EmailMessageData{InReplyTo: []string{ + "", "", + }}, + want: "", + }, + { + name: "Reply-To alone is not a parent", + msg: &models.EmailMessageData{ReplyTo: []string{"reports@example.net"}}, + want: "", + }, + { + name: "Reply-To never overrides In-Reply-To", + msg: &models.EmailMessageData{ + InReplyTo: []string{""}, + ReplyTo: []string{"someone@example.net"}, + }, + want: "", + }, + { + name: "a message that answers nothing has no parent", + msg: &models.EmailMessageData{}, + want: "", + }, + { + name: "nil is not a panic", + msg: nil, + want: "", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := threadParentID(tc.msg); got != tc.want { + t.Errorf("threadParentID() = %q, want %q", got, tc.want) + } + }) + } +} + +// Two unrelated messages from one sender must not share a parent, which is the +// shape of the bug: same Reply-To, different conversations. +func TestThreadParentIDKeepsUnrelatedMessagesApart(t *testing.T) { + sender := []string{"Aggregate Reports "} + first := &models.EmailMessageData{MessageID: "", ReplyTo: sender} + second := &models.EmailMessageData{MessageID: "", ReplyTo: sender} + + if p := threadParentID(first); p != "" { + t.Fatalf("first message got parent %q, want none", p) + } + if p := threadParentID(second); p != "" { + t.Fatalf("second message got parent %q, want none", p) + } +}