From 5c1ff4bb152bcb825cfa1cb222f4baaaca125747 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 15:33:36 +0200 Subject: [PATCH] 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 | 9 +++-- .../mesh-mail/cmd/mesh-mail/search_test.go | 33 +++++++++++++++++++ modules/mesh-mail/cmd/mesh-mail/store.go | 24 +++++++++++--- 3 files changed, 59 insertions(+), 7 deletions(-) diff --git a/modules/mesh-mail/cmd/mesh-mail/pgstore.go b/modules/mesh-mail/cmd/mesh-mail/pgstore.go index 6bee5550..19832e74 100644 --- a/modules/mesh-mail/cmd/mesh-mail/pgstore.go +++ b/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) { diff --git a/modules/mesh-mail/cmd/mesh-mail/search_test.go b/modules/mesh-mail/cmd/mesh-mail/search_test.go index 5b19dbfb..62f8fce5 100644 --- a/modules/mesh-mail/cmd/mesh-mail/search_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/search_test.go @@ -819,3 +819,36 @@ func TestSearchWithNoFolderAndLimitAnswersDistinctMailsInOrder(t *testing.T) { sameOrder(t, got, "", "", "", "", "") }) } + +// 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: "", 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: "", 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: "", 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) + } + }) +} diff --git a/modules/mesh-mail/cmd/mesh-mail/store.go b/modules/mesh-mail/cmd/mesh-mail/store.go index 8e9c6d24..2a1d6784 100644 --- a/modules/mesh-mail/cmd/mesh-mail/store.go +++ b/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.