diff --git a/modules/mesh-mail-notifier/cmd/mesh-mail-notifier/notifier.go b/modules/mesh-mail-notifier/cmd/mesh-mail-notifier/notifier.go index 27e0d88d..e4fa191a 100644 --- a/modules/mesh-mail-notifier/cmd/mesh-mail-notifier/notifier.go +++ b/modules/mesh-mail-notifier/cmd/mesh-mail-notifier/notifier.go @@ -4,10 +4,12 @@ package main // issue 800 part B). It hears mesh-mail's added events on its own account (runs-as, ADR 0376 §2) and shows each // new inbox mail at most once, through node-notifier.send: quiet during a first sync (mesh-mail already says // nothing then, imap.go ~387), for mail from the account's own address, for mail already marked read (\Seen), -// and for anything outside the inbox. Of one account's qualifying mail within a sliding minute, the first -// three are shown one by one; the rest of that minute's mail is folded into one notice naming the count and -// the latest sender, rather than shown one by one (novox/hq ADR 0378 §9, the lead's reading, review round 1 of -// issue 800). +// and for anything outside the inbox. Of one account's qualifying mail, a mail is shown at once when fewer +// than FoldOver of that account's mails were shown in the Window before it; otherwise it is folded and shown, +// with the other mail its session folds, once Window has passed since the first of them — the window slides +// on the gap between mails, not on time elapsed since the session's first one, so a session already past its +// quota stays folding for as long as mail keeps arriving within Window of the last (novox/hq ADR 0378 §9, the +// lead's reading, review rounds 1 and 2 of issue 800). // // Nothing here names Betterbird (ADR 0379 §3). @@ -29,9 +31,14 @@ const SeenFlag = "\\Seen" // that window is folded into a single notice (novox/hq ADR 0378 §9: "more than three"). const FoldOver = 3 -// Window is the sliding minute ADR 0378 §9 judges a burst over: an account's first qualifying mail opens it, -// and everything that account qualifies for until it closes, FoldOver mails onward, is one window's count. -// There is no separate quiet-gap timer (review round 1 of issue 800: BurstWindow goes). +// Window is the sliding minute ADR 0378 §9 judges a burst over. It slides on the gap between one qualifying +// mail and the next, not on time elapsed since a session's first mail (review round 2 of issue 800): a mail +// is shown at once when fewer than FoldOver of the account's mails were shown in the Window before it; past +// that, it is folded and shown, with whatever else its session folds, once Window has passed since the first +// of them. A session's count never resets while mail keeps arriving within Window of the last one, however +// long the session has run; it is only ever replaced once idle for a full Window with nothing left pending +// (review round 2 of issue 800, BLOCKING: a buffer is never replaced while it holds an unsent mail). There is +// no separate quiet-gap timer beside this (review round 1 of issue 800: BurstWindow goes). const Window = time.Minute // RetryBackoff is how long the notifier waits, after an account's notice fails to send, before it tries that @@ -140,14 +147,18 @@ type AccountStatus struct { Summary string `json:"summary"` } -// accountBuffer is one account's sliding window: the first FoldOver qualifying mails are shown one by one as -// soon as Flush runs; the rest, from the fourth onward, are folded and shown as one notice once the window has -// run its Minute (novox/hq ADR 0378 §9, the lead's reading). +// accountBuffer is one account's sliding session: it stays open, and its count never resets, as long as a +// qualifying mail keeps arriving within Window of the previous one (review round 2 of issue 800). The first +// FoldOver mails of a session are shown one by one as soon as Flush runs; the rest, from the fourth onward, +// are folded and shown as one notice once Window has passed since the first of them, whether or not the +// session itself is still open. type accountBuffer struct { - windowStart time.Time - count int // qualifying mails counted in this window so far - immediate []EventBody - folded []EventBody + lastArrival time.Time // the latest qualifying mail's arrival: a gap of Window since this lets a fresh session open + foldStart time.Time // when this session's first folded mail arrived; zero while nothing is folded + count int // every qualifying mail this session has counted, shown or folded alike (bullet 3: the + // fold notice names this total, not just the overflow) + immediate []EventBody + folded []EventBody } // Notifier buffers qualifying mail per account and shows it: the first FoldOver mails of a window one by one, @@ -218,15 +229,25 @@ func (n *Notifier) Add(e EventBody) error { } now := n.Now() b := n.pending[e.Account] - if b == nil || now.Sub(b.windowStart) >= Window { - b = &accountBuffer{windowStart: now} + // A session stays open, and its count never resets, as long as a qualifying mail keeps arriving within + // Window of the last one — even once more than Window has passed since the session's own first mail + // (the sliding minute, review round 2 of issue 800). It is replaced only once it is both idle for a full + // Window and fully flushed: nothing pending is ever discarded to make room for a fresh session (BLOCKING: + // a buffer is never replaced while it holds an unsent mail — a failed send waiting for its retry, or a + // folded mail waiting for its minute's end). + if b == nil || (now.Sub(b.lastArrival) >= Window && len(b.immediate) == 0 && len(b.folded) == 0) { + b = &accountBuffer{} n.pending[e.Account] = b } + b.lastArrival = now b.count++ n.buffered[handle] = true if b.count <= FoldOver { b.immediate = append(b.immediate, e) } else { + if b.foldStart.IsZero() { + b.foldStart = now + } b.folded = append(b.folded, e) } return nil @@ -299,8 +320,10 @@ func (n *Notifier) Flush() error { return nil } -// flushAccount ships one account's ready notices (its immediate mail, and its folded mail once the window is -// over), and keeps whatever fails to send pending, under cooldown, for the next Flush. +// flushAccount ships one account's ready notices (its immediate mail, and its folded mail once Window has +// passed since the first of them), and keeps whatever fails to send pending, under cooldown, for the next +// Flush. Nothing is deleted before it is shown (review round 1 BLOCKING 2), and nothing is replaced, or lost, +// while it is still pending (review round 2 BLOCKING; see Add). func (n *Notifier) flushAccount(account string, now time.Time) { n.mu.Lock() if cd, ok := n.cooldown[account]; ok && now.Before(cd) { @@ -313,17 +336,19 @@ func (n *Notifier) flushAccount(account string, now time.Time) { return } immediate := append([]EventBody(nil), b.immediate...) - windowDone := now.Sub(b.windowStart) >= Window + // The fold group is due Window after its own first mail — independent of whether the session itself is + // still open (review round 2 of issue 800: the window slides on gaps, so a session can still be taking in + // new mail well past its first fold's deadline). + foldDue := !b.foldStart.IsZero() && now.Sub(b.foldStart) >= Window var fold []EventBody - if windowDone { + foldCount := b.count // every mail of the account this session has counted (bullet 3), not just the fold's own + if foldDue { fold = append([]EventBody(nil), b.folded...) } n.mu.Unlock() if len(immediate) == 0 && len(fold) == 0 { - if windowDone { - n.closeIfEmpty(account, b) - } + n.closeIfIdle(account, b, now) return } @@ -338,7 +363,7 @@ func (n *Notifier) flushAccount(account string, now time.Time) { } foldSent := false if failErr == nil && len(fold) > 0 { - if err := n.show(n.foldNotice(account, fold)); err != nil { + if err := n.show(n.foldNotice(account, fold, foldCount)); err != nil { failErr = err } else { foldSent = true @@ -349,13 +374,19 @@ func (n *Notifier) flushAccount(account string, now time.Time) { if cur := n.pending[account]; cur == b { cur.immediate = cur.immediate[sentImmediate:] if foldSent { - cur.folded = nil + cur.folded = cur.folded[len(fold):] + if len(cur.folded) == 0 { + cur.foldStart = time.Time{} + } else { + // More mail folded in while this batch was being sent: its own Window starts now, counted from + // when it was first noticed. Nothing of it is lost either way. + cur.foldStart = now + } } - // The window itself — its start and count — is only ever dropped once it has run its course: while it - // is still open, mail arriving later in it must still count against the same window, even if nothing - // is pending to send right this moment (the bug review round 1 of issue 800 found: emptying the - // buffer early reset the window and let a fourth mail be shown on its own instead of folded). - if windowDone && len(cur.immediate) == 0 && len(cur.folded) == 0 { + // The session itself — its lastArrival and count — is only ever dropped once it has gone idle for a + // full Window with nothing left pending (review round 2 of issue 800, BLOCKING: a buffer is never + // replaced, or its unsent mail lost, while it still holds one). + if now.Sub(cur.lastArrival) >= Window && len(cur.immediate) == 0 && len(cur.folded) == 0 { delete(n.pending, account) } } @@ -369,10 +400,13 @@ func (n *Notifier) flushAccount(account string, now time.Time) { n.mu.Unlock() } -func (n *Notifier) closeIfEmpty(account string, b *accountBuffer) { +// closeIfIdle drops an account's session once it has gone idle for a full Window with nothing left pending: +// the next qualifying mail then opens a fresh session with a fresh quota (see Add). +func (n *Notifier) closeIfIdle(account string, b *accountBuffer, now time.Time) { n.mu.Lock() defer n.mu.Unlock() - if cur := n.pending[account]; cur == b && len(cur.immediate) == 0 && len(cur.folded) == 0 { + if cur := n.pending[account]; cur == b && now.Sub(cur.lastArrival) >= Window && + len(cur.immediate) == 0 && len(cur.folded) == 0 { delete(n.pending, account) } } @@ -418,15 +452,17 @@ func (n *Notifier) singleNotice(account string, e EventBody) Notice { } } -// foldNotice is one notice for everything an account's window folded — its fourth qualifying mail onward — -// naming how many and the latest sender (novox/hq ADR 0378 §9: "more than three new mails from one account -// within a minute become one notice that says how many and names the latest sender"). -func (n *Notifier) foldNotice(account string, events []EventBody) Notice { +// foldNotice is one notice for everything an account's session folded — its fourth qualifying mail onward — +// naming the latest sender and how many mail the account has had in total this session, not only the +// overflow (novox/hq ADR 0378 §9: "more than three new mails from one account within a minute become one +// notice that says how many and names the latest sender"; review round 2 of issue 800, bullet 3: the count is +// every mail of the account in that minute, "mail" for one and "mails" for more). +func (n *Notifier) foldNotice(account string, events []EventBody, count int) Notice { latest := events[len(events)-1] notice := Notice{ Account: account, Args: map[string]any{ - "summary": strconv.Itoa(len(events)) + " new mails in " + account, + "summary": strconv.Itoa(count) + " new " + mailWord(count) + " in " + account, "body": "latest from " + latest.sender(), "app_name": "mesh-mail", }, @@ -438,6 +474,14 @@ func (n *Notifier) foldNotice(account string, events []EventBody) Notice { return notice } +// mailWord says "mail" for a count of one, "mails" for anything else (review round 2 of issue 800, bullet 3). +func mailWord(count int) string { + if count == 1 { + return "mail" + } + return "mails" +} + // show sends one notice, marks every message it accounts for seen, keeps the account's status, and emits // `shown`. Nothing is marked, kept or emitted when Send fails: the caller keeps the notice pending instead. func (n *Notifier) show(notice Notice) error { diff --git a/modules/mesh-mail-notifier/cmd/mesh-mail-notifier/notifier_test.go b/modules/mesh-mail-notifier/cmd/mesh-mail-notifier/notifier_test.go index 5a191ad6..841dd86a 100644 --- a/modules/mesh-mail-notifier/cmd/mesh-mail-notifier/notifier_test.go +++ b/modules/mesh-mail-notifier/cmd/mesh-mail-notifier/notifier_test.go @@ -246,8 +246,8 @@ func TestABurstShowsTheFirstThreeAtOnceAndFoldsTheRestAtTheWindowsEnd(t *testing if len(sent) != 4 { t.Fatalf("after the window ended, sent %d notice(s), want 4 (the folded fourth mail too): %+v", len(sent), sent) } - if sent[3]["summary"] != "1 new mails in home" || !strings.Contains(sent[3]["body"].(string), "Sender 4") { - t.Errorf("the fold notice is %+v, want it to say 1 new mail in home, naming Sender 4", sent[3]) + if sent[3]["summary"] != "4 new mails in home" || !strings.Contains(sent[3]["body"].(string), "Sender 4") { + t.Errorf("the fold notice is %+v, want it to count every mail of the session (4), naming Sender 4", sent[3]) } } @@ -298,8 +298,8 @@ func TestARedeliveredEventIsNeverShownOrCountedTwice(t *testing.T) { if len(sent) != 4 { t.Fatalf("sent %d notice(s), want 4 (one, mail 2, mail 3, and one fold notice): %+v", len(sent), sent) } - if sent[3]["summary"] != "1 new mails in home" { - t.Errorf("the fold notice is %+v, want it to count the redelivered fourth mail once, not twice", sent[3]) + if sent[3]["summary"] != "4 new mails in home" { + t.Errorf("the fold notice is %+v, want it to count the redelivered fourth mail once, not twice, among the session's 4", sent[3]) } } @@ -394,3 +394,143 @@ func TestQualifyingRulesStillHold(t *testing.T) { t.Fatalf("sent %+v, want nothing: an already-read mail, the account's own mail and mail outside the inbox never qualify", sent) } } + +// TestABufferIsNeverReplacedWhileItHoldsAnUnsentMail is review round 2 of issue 800's first test (BLOCKING): +// a send that fails keeps its mail pending; once the cooldown has passed, but before the next Flush retries +// it, a new qualifying mail arrives. Neither mail is lost: both are shown once the retry succeeds. +func TestABufferIsNeverReplacedWhileItHoldsAnUnsentMail(t *testing.T) { + clock := &fakeClock{now: time.Date(2026, 10, 15, 12, 0, 0, 0, time.UTC)} + store := newFakeStore() + var sent []map[string]any + failSend := true + n := &Notifier{ + Now: clock.Now, + Send: func(args map[string]any) error { + if failSend { + return errors.New("node-notifier.send: no answer") + } + sent = append(sent, args) + return nil + }, + Accounts: func() (map[string]string, error) { return map[string]string{}, nil }, + Store: store, + EmitShown: func(string, string) error { return nil }, + Logf: t.Logf, + } + + account := "home" + if err := n.Add(mail(account, 1, "m1", "Mail 1", "Sender 1")); err != nil { + t.Fatalf("Add mail 1: %v", err) + } + if err := n.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if len(sent) != 0 { + t.Fatalf("sent %d notice(s), want 0: the send failed", len(sent)) + } + + // The cooldown passes, but no retry has happened yet (Flush is not called again before the new mail + // arrives): a buffer this old must not be thrown away for a fresh one. + clock.Advance(RetryBackoff) + if err := n.Add(mail(account, 2, "m2", "Mail 2", "Sender 2")); err != nil { + t.Fatalf("Add mail 2: %v", err) + } + + failSend = false + if err := n.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if len(sent) != 2 { + t.Fatalf("sent %d notice(s), want 2 (mail 1's retry and mail 2, neither lost): %+v", len(sent), sent) + } + for i, want := range []string{"Mail 1", "Mail 2"} { + if sent[i]["summary"] != want { + t.Errorf("sent[%d] = %+v, want summary %q", i, sent[i], want) + } + } +} + +// TestAFoldedMailIsNeverLostBetweenTheWindowsEndAndTheNextFlush is review round 2 of issue 800's second test +// for the same BLOCKING finding: no failure here, but the fourth mail is folded, its minute ends, and a new +// mail arrives before the next Flush. The folded mail is still shown, never lost. +func TestAFoldedMailIsNeverLostBetweenTheWindowsEndAndTheNextFlush(t *testing.T) { + clock := &fakeClock{now: time.Date(2026, 10, 15, 12, 0, 0, 0, time.UTC)} + store := newFakeStore() + var sent []map[string]any + n := newTestNotifier(t, clock, store, &sent) + + account := "home" + for i := 1; i <= 4; i++ { + if err := n.Add(mail(account, i, fmt.Sprintf("m%d", i), fmt.Sprintf("Mail %d", i), fmt.Sprintf("Sender %d", i))); err != nil { + t.Fatalf("Add mail %d: %v", i, err) + } + } + if err := n.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if len(sent) != 3 { + t.Fatalf("sent %d notice(s), want 3 shown one by one, the fourth folded, not yet due: %+v", len(sent), sent) + } + + // The fold's minute is over (a Window since mail 4, its first folded mail), but Flush has not run again + // yet: a new mail arrives in that gap. + clock.Advance(Window + time.Second) + if err := n.Add(mail(account, 5, "m5", "Mail 5", "Sender 5")); err != nil { + t.Fatalf("Add mail 5: %v", err) + } + + if err := n.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if len(sent) != 4 { + t.Fatalf("sent %d notice(s), want 4 (one fold notice carrying both mail 4 and mail 5): %+v", len(sent), sent) + } + if sent[3]["summary"] != "5 new mails in home" || !strings.Contains(sent[3]["body"].(string), "Sender 5") { + t.Errorf("the fold notice is %+v, want it to count all 5 of the session, naming Sender 5 (the latest)", sent[3]) + } +} + +// TestTheWindowSlidesOnGapsNotOnElapsedTimeSinceItsFirstMail is review round 2 of issue 800's test for the +// sliding minute: a mail is shown at once when fewer than three of the account's mails were shown in the 60 +// seconds before it; otherwise it is folded and shown with the rest 60 seconds after the first of them. Mails +// at 0, 50, 55, 70, 75 and 80 seconds: 0, 50 and 55 are each within 60s of fewer than 3 prior mails, so each +// is shown; 70 is not (50 and 55 both fall in its preceding 60s, and the session is already past its quota), +// so it and everything after it, within the same ongoing session, fold into one notice. +func TestTheWindowSlidesOnGapsNotOnElapsedTimeSinceItsFirstMail(t *testing.T) { + clock := &fakeClock{now: time.Date(2026, 10, 15, 12, 0, 0, 0, time.UTC)} + store := newFakeStore() + var sent []map[string]any + n := newTestNotifier(t, clock, store, &sent) + + account := "home" + deltas := []time.Duration{0, 50 * time.Second, 5 * time.Second, 15 * time.Second, 5 * time.Second, 5 * time.Second} + for i, d := range deltas { + clock.Advance(d) + if err := n.Add(mail(account, i+1, fmt.Sprintf("m%d", i+1), fmt.Sprintf("Mail %d", i+1), fmt.Sprintf("Sender %d", i+1))); err != nil { + t.Fatalf("Add mail %d: %v", i+1, err) + } + if err := n.Flush(); err != nil { + t.Fatalf("Flush at mail %d: %v", i+1, err) + } + } + if len(sent) != 3 { + t.Fatalf("sent %d notice(s) before the fold's window ends, want 3 shown one by one (0s, 50s, 55s): %+v", len(sent), sent) + } + for i, want := range []string{"Mail 1", "Mail 2", "Mail 3"} { + if sent[i]["summary"] != want { + t.Errorf("sent[%d] = %+v, want summary %q", i, sent[i], want) + } + } + + // The fold group's own window — a minute from its first mail, at 70s — ends at 130s. + clock.Advance(50 * time.Second) + if err := n.Flush(); err != nil { + t.Fatalf("Flush: %v", err) + } + if len(sent) != 4 { + t.Fatalf("sent %d notice(s), want 4 (70s, 75s and 80s folded into one, at 130s): %+v", len(sent), sent) + } + if sent[3]["summary"] != "6 new mails in home" || !strings.Contains(sent[3]["body"].(string), "Sender 6") { + t.Errorf("the fold notice is %+v, want it to count all 6 of the session, naming Sender 6 (80s, the latest)", sent[3]) + } +}