mesh-mail: dedup-archive picks the same copy in both stores when a message-id also sits in a second non-archive folder (issue 1248, review round 1 finding 1)

modules/mesh-mail/cmd/mesh-mail/pgstore.go

@@ -578,8 +578,11 @@ func orderExpr(orderBy string) string {
 // account and message_id, except an empty message_id (AC5) is never grouped with anything else — each such row
 // gets its own one-row group, keyed by its own (account, folder, uid) instead, which is already unique. Within a
 // group the copy outside archiveFolderNames wins (AC1); only when every copy is archived does the archive copy
-// answer (AC2). Callers skip this (keep the plain "message where ...") when a folder is named (AC3): every mail
-// of that folder answers, never merged with anything else.
+// answer (AC2). When a message-id sits in two non-archive folders too (e.g. INBOX and a label folder such as
+// "INBOX/Telenet"; review round 1 finding 1), folder ascending then uid ascending breaks the tie — the same
+// fixed rule memStore's dedupeBetter (store.go) uses, so the two stores never pick a different copy. Callers
+// skip this (keep the plain "message where ...") when a folder is named (AC3): every mail of that folder
+// answers, never merged with anything else.
 func dedupedMessagesSQL(whereSQL string, param func(any) string) string {
 	archiveParam := param(lowerStrings(archiveFolderNames))
 	return `(select distinct on (account, dedup_key) ` + messageColumns + ` from (
@@ -587,7 +590,7 @@ func dedupedMessagesSQL(whereSQL string, param func(any) string) string {
 			case when message_id = '' then 'u:' || folder || ':' || uid::text else message_id end as dedup_key,
 			(lower(folder) = any(` + archiveParam + `)) as dedup_archived
 		from message where ` + whereSQL + `
-	) m order by account, dedup_key, dedup_archived asc, uid asc) message`
+	) m order by account, dedup_key, dedup_archived asc, folder asc, uid asc) message`
 }
 
 func (s *PgStore) Search(ctx context.Context, q Query) ([]Message, error) {

modules/mesh-mail/cmd/mesh-mail/search_test.go

@@ -819,3 +819,36 @@ func TestSearchWithNoFolderAndLimitAnswersDistinctMailsInOrder(t *testing.T) {
 		sameOrder(t, got, "<many6@dup>", "<many5@dup>", "<many4@dup>", "<many3@dup>", "<many2@dup>")
 	})
 }
+
+// 1248/AC1 (review round 1, finding 1): the same message-id sits in two non-archive folders (INBOX and a label
+// folder, "INBOX/Telenet", as gmail keeps a label as a second folder) as well as the archive. Searched with no
+// folder, it is still answered once, and from INBOX — the fixed tie-break (non-archive before archive, then
+// folder ascending, then uid ascending) picks "INBOX" over "INBOX/Telenet" because it sorts first, never by
+// whichever copy a Go map's randomised iteration happened to meet first.
+func TestSearchWithNoFolderPicksTheSameCopyAsPostgresAmongTwoNonArchiveFolders(t *testing.T) {
+	eachStore(t, func(t *testing.T, st Store) {
+		ctx := context.Background()
+		for _, m := range []Message{
+			{Account: "labeled", Folder: "INBOX", UID: 1, MessageID: "<labeled1@dup>", Subject: "labeled",
+				Date: at1248(9, 0), From: []Person{{Address: "a@example.org"}}, To: []Person{}, Flags: []string{}, Attachments: []AttachmentRef{}},
+			{Account: "labeled", Folder: "INBOX/Telenet", UID: 1, MessageID: "<labeled1@dup>", Subject: "labeled",
+				Date: at1248(9, 0), From: []Person{{Address: "a@example.org"}}, To: []Person{}, Flags: []string{}, Attachments: []AttachmentRef{}},
+			{Account: "labeled", Folder: "[Gmail]/All Mail", UID: 1, MessageID: "<labeled1@dup>", Subject: "labeled",
+				Date: at1248(9, 0), From: []Person{{Address: "a@example.org"}}, To: []Person{}, Flags: []string{}, Attachments: []AttachmentRef{}},
+		} {
+			if err := st.Add(ctx, m, nil); err != nil {
+				t.Fatal(err)
+			}
+		}
+		got, err := st.Search(ctx, Query{Accounts: []string{"labeled"}, Limit: 10})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(got) != 1 {
+			t.Fatalf("labeled1@dup answered %d time(s): %+v", len(got), got)
+		}
+		if got[0].Folder != "INBOX" {
+			t.Fatalf("got folder %q, want INBOX", got[0].Folder)
+		}
+	})
+}

modules/mesh-mail/cmd/mesh-mail/store.go

@@ -627,9 +627,10 @@ func isArchiveFolder(folder string) bool {
 // dedupeArchive is 1248/AC1, AC2, AC4-AC7: ms (already matching a Query with no Folder named) with each
 // (account, message-id) kept once — the copy outside archiveFolderNames, or the archive copy when that is the
 // only copy. An empty message-id (AC5) is never merged with anything: every such mail is its own group, keyed by
-// its own (account, folder, uid), which msgKey already makes unique. The order of ms is otherwise kept
-// (Search/GroupBySender sort or group afterwards), so which duplicate "wins" a tie between two non-archive
-// copies (never asked for by an acceptance criterion) is decided by content, not by position.
+// its own (account, folder, uid), which msgKey already makes unique. Which copy wins when a message-id sits in
+// two non-archive folders too (e.g. INBOX and a label folder such as "INBOX/Telenet"; review round 1 finding 1)
+// is decided by dedupeBetter's fixed rule, the same one dedupedMessagesSQL (pgstore.go) uses, never by the order
+// ms happens to arrive in — which, from a Go map, is randomised per run.
 func dedupeArchive(ms []Message) []Message {
 	type groupKey struct{ account, id string }
 	kept := map[groupKey]int{} // groupKey -> index into out
@@ -641,7 +642,7 @@ func dedupeArchive(ms []Message) []Message {
 		}
 		k := groupKey{m.Account, id}
 		if i, ok := kept[k]; ok {
-			if isArchiveFolder(out[i].Folder) && !isArchiveFolder(m.Folder) {
+			if dedupeBetter(m, out[i]) {
 				out[i] = m
 			}
 			continue
@@ -652,6 +653,21 @@ func dedupeArchive(ms []Message) []Message {
 	return out
 }
 
+// dedupeBetter is 1248 (review round 1, finding 1): whether a is the copy dedupeArchive should keep over b,
+// within one (account, message-id) group — non-archive before archive, then folder ascending, then uid
+// ascending, the same order dedupedMessagesSQL's "order by ... dedup_archived, folder, uid" picks in Postgres,
+// so the two stores never answer a different copy of the same mail.
+func dedupeBetter(a, b Message) bool {
+	aArchived, bArchived := isArchiveFolder(a.Folder), isArchiveFolder(b.Folder)
+	if aArchived != bArchived {
+		return !aArchived
+	}
+	if a.Folder != b.Folder {
+		return a.Folder < b.Folder
+	}
+	return a.UID < b.UID
+}
+
 // wroteToByAccount is 1231/AC14's own data: for each account, every address (lowercased) that account has
 // written to (as a "to" or a "cc") from a folder named one of sentFolderNames. Computed only when needed (asked
 // is false otherwise), since it walks every message.