fix(mail): den samme mail blev en opgave igen og igen

Rapporteret fra drift med et skærmbillede af den samme mail seks gange i træk.

**IMAP: flaget kommer af, når mailen er blevet en opgave.** Markøren var et uid —
husk det højeste, begynd over det næste gang — og den holdt kun, hvis uids var,
hvad serveren sagde. Den brød på et fetch-svar uden uid, på en UIDVALIDITY, der
skiftede under en genopbygget postkasse, og på to postkasse-rækker, der pegede
samme sted og hver især førte deres eget regnskab. Alle tre gav det samme
symptom.

Så regnskabet flytter derhen, hvor spørgsmålet bliver stillet. "Flaget" betyder
"lav det her om til en opgave"; når den er en opgave, har flaget gjort sit
arbejde og er ikke sandt længere. Så er der ingenting at huske, og dermed
ingenting, der kan komme ud af trit — en anden synkronisering, en anden
postkasse-række eller en database gendannet fra en kopi finder alle det samme
svar, fordi svaret ligger i postkassen.

Flaget tages af *efter* opgaven er skrevet. Et flag fjernet fra en mail, hvis
opgave aldrig blev gemt, er en mail, ingen ser igen.

**Gmail havde den samme fejl af en anden grund.** Listen over allerede behandlede
beskeder blev bygget om ved hver kørsel af *kun de id'er, Gmail svarede med den
gang* — for at den ikke skulle vokse uendeligt. Men forespørgslen er begrænset,
så på en konto med mere stjernemarkeret post, end der er plads til i ét svar,
glemte hver kørsel det, den forrige havde husket. De kom tilbage som ubehandlede
og blev opgaver igen. Det var ikke en grænse, det var en måde at glemme på.
Listen har nu et loft på antal i stedet, nyeste først.

Gmail kan ikke få taget stjernen af: scopet er `gmail.readonly`, med vilje, fordi
det er dét, der gør samtykkeskærmen til noget, et menneske kan sige ja til. Derfor
skal regnskabet blive på vores side dér — men det skal så være et regnskab, der
kan holde.

Prøven kører hele vejen mod en rigtig IMAP-server: find de flagede, tag flaget af,
og spørg så forfra med markøren sat til nul — altså det værst tænkelige tilfælde,
hvor alt, vi selv havde husket, er væk. Der kommer ingenting tilbage.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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 {

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

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
+}

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")
+	}
+}