diff --git a/internal/httpapi/gmail_handlers.go b/internal/httpapi/gmail_handlers.go index 74dc370..b5b403d 100644 --- a/internal/httpapi/gmail_handlers.go +++ b/internal/httpapi/gmail_handlers.go @@ -251,6 +251,17 @@ func (s *Server) handleGmailSyncNow(w http.ResponseWriter, r *http.Request) { // // Returns how many were made. Also used by the background job, which is why it // takes a context and a user rather than a request. +// maxGmailSeen bounds the list of message ids already turned into tasks. +// +// Gmail is read-only here — the scope is `gmail.readonly`, deliberately, because +// asking for less is what makes the consent screen something a person can agree +// to — so verdande cannot unstar a message the way the IMAP side unflags one. The +// record has to live on this side, and a record on this side has to be bounded. +// +// A count rather than the previous rule of "whatever Gmail returned this run", +// which was not a bound but a way of forgetting. +const maxGmailSeen = 2000 + func (s *Server) SyncGmail(ctx context.Context, user *store.User) (int, error) { box, err := s.db.MailboxOfKind(ctx, user.ID, "gmail") if err != nil { @@ -362,14 +373,35 @@ func (s *Server) SyncGmail(ctx context.Context, user *store.User) (int, error) { } if created > 0 { - // Only the ids Gmail still returns are kept, so the list cannot grow - // without bound as messages fall out of the 30-day window. - keep := make([]string, 0, len(ids)) + // The ids this run saw, on top of the ones already known. + // + // It used to keep only the ids Gmail returned *this run*, to stop the list + // growing without bound. That threw away every id that was not on the page + // it happened to fetch — and the query is capped, so on an account with more + // starred mail than one page, each run forgot the messages the last one had + // remembered. They came back unseen, and became tasks again, and again. That + // is the duplicate. + // + // Bounded by a count instead. Newest first, and a ceiling far above any + // plausible page, so an id leaves the list because it is old rather than + // because it was not in the last answer. + keep := make([]string, 0, len(ids)+len(box.Seen)) + added := map[string]bool{} for _, id := range ids { - if seen[id] { + if seen[id] && !added[id] { keep = append(keep, id) + added[id] = true } } + for _, id := range box.Seen { + if !added[id] { + keep = append(keep, id) + added[id] = true + } + } + if len(keep) > maxGmailSeen { + keep = keep[:maxGmailSeen] + } box.Seen = keep box.LastSyncAt = time.Now() if err := s.db.SaveMailbox(ctx, box); err != nil { diff --git a/internal/httpapi/mailbox_handlers.go b/internal/httpapi/mailbox_handlers.go index 8baaeab..6c87541 100644 --- a/internal/httpapi/mailbox_handlers.go +++ b/internal/httpapi/mailbox_handlers.go @@ -175,6 +175,10 @@ func (s *Server) SyncMailbox(ctx context.Context, user *store.User, m *store.Mai created := 0 highest := m.LastUID + // The uids of the mails that actually became tasks. Only those get unflagged, + // and only after the task is committed: a flag removed from a mail whose task + // was never written is a mail nobody will ever see again. + done := make([]uint32, 0, len(messages)) for _, msg := range messages { if ctx.Err() != nil { // What was made so far is already committed and counted, and the marker @@ -204,12 +208,27 @@ func (s *Server) SyncMailbox(ctx context.Context, user *store.User, m *store.Mai continue } created++ + done = append(done, msg.UID) if msg.UID > highest { highest = msg.UID } s.hub.Publish(projectID, "task.created", toTaskJSON(*task)) } + // The flag comes off what became a task. This is what actually stops a mail + // arriving twice; the marker below is now a second line rather than the only + // one. See imap.Unflag for why the record belongs in the mailbox and not here. + // + // Outside the request's context, like the marker: a cancelled request that + // already created tasks must still take the flags off them, or the next run + // makes them all again. + if err := client.Unflag(done); err != nil { + // Not fatal, and not the caller's problem. A folder somebody opened + // read-only, or a server that refuses the STORE, still gets the uid marker + // underneath — worse, but not broken. + s.log.Warn("could not unflag imported mail", "err", err, "mailbox", m.ID, "count", len(done)) + } + // A run that got all the way through moves the marker to the whole of what the // search considered, not merely to the last message it happened to read. That // is what stops the same mail becoming a task again on the next run when the diff --git a/internal/imap/imap.go b/internal/imap/imap.go index cd1465e..c47d140 100644 --- a/internal/imap/imap.go +++ b/internal/imap/imap.go @@ -310,3 +310,61 @@ func cut(text string) string { } return text } + +// Unflag takes the flag off the messages that have become tasks. +// +// This is what stops the same mail arriving twice, and it replaces a marker that +// could not be made to hold. The marker was a uid: remember the highest one seen, +// start above it next time. That is correct only if uids are what the server says +// they are, and it broke in every way such a scheme can — a fetch reply without a +// uid, a UIDVALIDITY that changed under a rebuilt mailbox, two mailbox rows +// pointing at the same folder and each keeping its own marker. Every one of those +// produced the same symptom: the same mail became a task again, and again. +// +// Unflagging moves the record out of our database and into the mailbox, where the +// question is asked. "Flagged" means "turn this into a task"; once it is a task, +// the flag has done its job and is not true any more. Nothing has to be +// remembered, so there is nothing to get out of step — a second sync, a second +// mailbox row, or a database restored from a backup all find the same answer, +// because the answer is in the mailbox. +// +// It is destructive in the small way that matters: it changes something in +// somebody's mail. That is the trade, and it is the right way round — a flag is a +// note to self that this needs doing, and it has been done. +// +// The folder is re-selected writable. Since() opens it read-only on purpose, and +// a search should not need write access just because a later step might. +func (c *Client) Unflag(uids []uint32) error { + if len(uids) == 0 { + return nil + } + if _, err := c.c.Select(c.folder, nil).Wait(); err != nil { + return fmt.Errorf("open %s for writing: %w", c.folder, err) + } + + set := imap.UIDSetNum() + for _, u := range uids { + if u == 0 { + // No uid means nothing to address. The message keeps its flag and will + // be seen again — which is the failure this whole function exists to + // prevent, so it is worth saying rather than skipping quietly. + continue + } + set.AddNum(imap.UID(u)) + } + if len(set) == 0 { + return nil + } + + // Silent: the server has nothing to tell us that we would act on, and the + // untagged replies would only have to be collected and thrown away. + cmd := c.c.Store(set, &imap.StoreFlags{ + Op: imap.StoreFlagsDel, + Silent: true, + Flags: []imap.Flag{imap.FlagFlagged}, + }, nil) + if err := cmd.Close(); err != nil { + return fmt.Errorf("unflag in %s: %w", c.folder, err) + } + return nil +} diff --git a/internal/imap/roundtrip_test.go b/internal/imap/roundtrip_test.go index 5eebe48..4009aa1 100644 --- a/internal/imap/roundtrip_test.go +++ b/internal/imap/roundtrip_test.go @@ -316,3 +316,81 @@ func TestNothingAtOrBelowTheMarkerComesBack(t *testing.T) { } } } + +// Den samme mail må ikke kunne blive en opgave to gange. +// +// Prøven kører hele vejen imod en rigtig server: den finder de flagede mails, +// tager flaget af dem, og spørger så forfra som en ny synkronisering ville — med +// markøren sat til nul, altså det værst tænkelige tilfælde, hvor alt, hvad vi +// selv havde husket, er væk. Der skal ikke komme noget tilbage. +// +// Det er det, markøren ikke kunne love. Den holdt kun, hvis uids var, hvad +// serveren sagde, og fejlede på et fetch-svar uden uid, på en UIDVALIDITY, der +// skiftede, og på to postkasse-rækker, der pegede samme sted og hver især førte +// deres eget regnskab. Alle tre gav det samme: mailen blev en opgave igen. +func TestAnUnflaggedMailDoesNotComeBack(t *testing.T) { + addr, pool := serveMailbox(t, []mail{ + {subject: "Faktura 4711", from: "anders@example.dk", body: "Vedhæftet.", flagged: true}, + {subject: "Nyhedsbrev", from: "noreply@example.com", body: "Læs mere", flagged: false}, + {subject: "Kontrakt", from: "jura@example.dk", body: "Til underskrift", flagged: true}, + }) + + client, err := Dial(Account{Host: addr, Username: "kw", Password: "hemmelig", RootCAs: pool}) + if err != nil { + t.Fatal(err) + } + defer client.Close() + + msgs, _, err := client.Since(0, 25) + if err != nil { + t.Fatal(err) + } + if len(msgs) != 2 { + t.Fatalf("fandt %d flagede mails, ventede 2", len(msgs)) + } + + uids := make([]uint32, 0, len(msgs)) + for _, m := range msgs { + uids = append(uids, m.UID) + } + if err := client.Unflag(uids); err != nil { + t.Fatalf("Unflag: %v", err) + } + + // Forfra, og fra nul: selv en instans, der intet husker, må ikke finde dem igen. + again, _, err := client.Since(0, 25) + if err != nil { + t.Fatal(err) + } + if len(again) != 0 { + t.Fatalf("%d mails kom igen efter at flaget var taget af — den første af dem er %q", + len(again), again[0].Subject) + } +} + +// Et tomt kald må ikke åbne mappen skrivbar for ingenting. +func TestUnflagWithNothingToDoIsQuiet(t *testing.T) { + addr, pool := serveMailbox(t, []mail{ + {subject: "Faktura", from: "a@example.dk", body: "x", flagged: true}, + }) + client, err := Dial(Account{Host: addr, Username: "kw", Password: "hemmelig", RootCAs: pool}) + if err != nil { + t.Fatal(err) + } + defer client.Close() + + if err := client.Unflag(nil); err != nil { + t.Errorf("Unflag(nil) = %v", err) + } + // Kun nuller: der er intet at adressere, og mailen skal beholde sit flag. + if err := client.Unflag([]uint32{0, 0}); err != nil { + t.Errorf("Unflag(nuller) = %v", err) + } + msgs, _, err := client.Since(0, 25) + if err != nil { + t.Fatal(err) + } + if len(msgs) != 1 { + t.Errorf("mailen mistede sit flag, uden at nogen havde adresseret den") + } +}