imap: support various provider policies for expunge calls

To delete N messages, aerc issues a single EXPUNGE command to the IMAP
server. The server sends back one ExpungeUpdate for each of those N
messages, and aerc reacts to each to update the folder contents.

Unfortunately, the RFC does not specify the order in which a conforming
server should send those "individual replies". aerc currently implicitly
assumes that it deletes messages in increasing sequence number order
(what GMail or FastMail do), and deleting N>1 messages will generally
not work for servers that use a different policy (e.g. Office 365 or
WorkMail, among others).

This patch implements an automatic detection of the policy used by the
server and adapts to it. Since some servers use a policy that can
confuse the automatic detection (e.g. WorkMail that deletes in random
order), it's also possible to statically configure that policy in
accounts.conf if needed.

Fixes: 80408384 ("handle outdated sequence numbers from server [...]")
Signed-off-by: Simon Martin <simon@nasilyan.com>
Tested-by: Karel Balej <balejk@matfyz.cz>
Acked-by: Robin Jarry <robin@jarry.cc>

doc/aerc-imap.5.scd

@@ -153,6 +153,18 @@ are available:
 
 	Default: _false_
 
+*expunge-policy* = _auto_|_low-to-high_|_stable_
+	Specifies the deletion policy used when deleting multiple messages in
+	one shot. _auto_ attempts to automatically detect it, and will be
+	correct most of the times. _low-to-high_ specifies that the server
+	deletes messages in increasing sequence number order (this is what GMail
+	or FastMail do, and will correctly handled by the automatic detection).
+	_stable_ specifies that the server does not mutate the sequence numbers
+	it received (this is what Dovecot or WorkMail do, and is *not* reliably
+	automatically detected).
+
+	Default: _auto_
+
 # SEE ALSO
 
 *aerc*(1) *aerc-accounts*(5)

worker/imap/configure.go

@@ -81,6 +81,7 @@ func (w *IMAPWorker) handleConfigure(msg *types.Configure) error {
 
 	w.config.cacheEnabled = false
 	w.config.cacheMaxAge = 30 * 24 * time.Hour // 30 days
+	w.config.expungePolicy = ExpungePolicyAuto
 
 	for key, value := range msg.Config.Params {
 		switch key {
@@ -161,6 +162,17 @@ func (w *IMAPWorker) handleConfigure(msg *types.Configure) error {
 				return fmt.Errorf("invalid use-gmail-ext value %v: %w", value, err)
 			}
 			w.config.useXGMEXT = val
+		case "expunge-policy":
+			switch value {
+			case "auto":
+				w.config.expungePolicy = ExpungePolicyAuto
+			case "low-to-high":
+				w.config.expungePolicy = ExpungePolicyLowToHigh
+			case "stable":
+				w.config.expungePolicy = ExpungePolicyStable
+			default:
+				return fmt.Errorf("invalid expunge-policy value %v", value)
+			}
 		}
 	}
 	if w.config.cacheEnabled {

worker/imap/expungehandler.go

@@ -0,0 +1,123 @@
+package imap
+
+import (
+	"sync"
+)
+
+// Provider dependent EXPUNGE handler
+//
+// To delete N items in a single command, aerc does the following:
+//   1. Set the \Deleted flag on those N items (identified by their sequence
+//      number)
+//   2. Call the EXPUNGE command
+// It then gets N ExpungeData messages from go-imap, that reference the
+// individual message actually deleted.
+// Unfortunately the IMAP RFC does not specify the order in which those
+// messages should be sent, and different providers have different policies.
+// In particular:
+//   - GMail and FastMail delete messages by increasing sequence number, and
+//     at each individual delete, decrement the sequence number of all the
+//     messages that still need to be deleted.
+//   - Office 365 deletes messages by decreasing sequence number.
+//   - Dovecot deletes messages in a seemingly random order.
+// The role of ExpungeHandler is to abstract out those differences, and
+// automatically adapt to the IMAP server's behaviour.
+// Since there's a non-zero probability that the automatic detection is wrong
+// if the server deletes items in a random order, it's also possible to
+// statically configure the expunge policy in accounts.conf.
+
+// The IMAP server behaviour when deleting multiple messages.
+const (
+	// Automatically detect behaviour from the first reply.
+	ExpungePolicyAuto = iota
+	// The server deletes message in increasing sequence number. After each
+	// delete, outstanding messages need to have their sequence numbers
+	// decremented.
+	ExpungePolicyLowToHigh
+	// The server deletes messages in any order, but does not change any of the
+	// sequence numbers.
+	ExpungePolicyStable
+)
+
+type ExpungeHandler struct {
+	lock     sync.Mutex
+	worker   *IMAPWorker
+	policy   int
+	items    map[uint32]uint32
+	minNum   uint32
+	gotFirst bool
+}
+
+// Create a new ExpungeHandler for a list of UIDs that are being deleted or
+// moved.
+func NewExpungeHandler(worker *IMAPWorker, uids []uint32) *ExpungeHandler {
+	snapshot, min := worker.seqMap.Snapshot(uids)
+	return &ExpungeHandler{
+		worker:   worker,
+		policy:   worker.config.expungePolicy,
+		items:    snapshot,
+		minNum:   min,
+		gotFirst: false,
+	}
+}
+
+// Translate the sequence number received from the IMAP server into the
+// associated UID, deduce the policy used by the server from the first reply,
+// and update the remaining mappings according to that policy if required.
+func (h *ExpungeHandler) PopSequenceNumber(seqNum uint32) (uint32, bool) {
+	h.lock.Lock()
+	defer h.lock.Unlock()
+	if !h.gotFirst {
+		h.gotFirst = true
+		logPrefix := "Configured"
+		// This is the very first reply we get; use it to infer the IMAP
+		// server's policy if the configuration asks us to.
+		if h.policy == ExpungePolicyAuto {
+			logPrefix = "Deduced"
+			if seqNum == h.minNum {
+				h.policy = ExpungePolicyLowToHigh
+			} else {
+				h.policy = ExpungePolicyStable
+			}
+		}
+		switch h.policy {
+		case ExpungePolicyLowToHigh:
+			h.worker.worker.Debugf("%s expunge policy: low-to-high", logPrefix)
+		case ExpungePolicyStable:
+			h.worker.worker.Debugf("%s expunge policy: stable", logPrefix)
+		}
+	}
+	// Resolve the UID from the sequence number and pop the expunger entry.
+	uid, ok := h.items[seqNum]
+	delete(h.items, seqNum)
+
+	// If the server uses the "low to high" policy, we need to decrement all
+	// the remaining entries since the server is doing the same on its end.
+	if ok && h.policy == ExpungePolicyLowToHigh {
+		newSeq := make(map[uint32]uint32)
+
+		for s, uid := range h.items {
+			newSeq[s-1] = uid
+		}
+
+		h.items = newSeq
+	}
+
+	if !ok {
+		h.worker.worker.Errorf("Unexpected sequence number; consider" +
+			"overriding the expunge-policy IMAP configuration")
+	}
+
+	return uid, ok
+}
+
+func (h *ExpungeHandler) IsExpunging(uid uint32) bool {
+	h.lock.Lock()
+	defer h.lock.Unlock()
+	for _, u := range h.items {
+		if u == uid {
+			return true
+		}
+	}
+	return false
+}

worker/imap/flags.go

@@ -49,6 +49,9 @@ func (imapw *IMAPWorker) handleDeleteMessages(msg *types.DeleteMessages) {
 	drain := imapw.drainUpdates()
 	defer drain.Close()
 
+	// Build provider-dependent EXPUNGE handler.
+	imapw.BuildExpungeHandler(models.UidToUint32List(msg.Uids))
+
 	item := imap.FormatFlagsOp(imap.AddFlags, true)
 	flags := []any{imap.DeletedFlag}
 	uids := toSeqSet(msg.Uids)

worker/imap/movecopy.go

@@ -3,6 +3,7 @@ package imap
 import (
 	"io"
 
+	"git.sr.ht/~rjarry/aerc/models"
 	"git.sr.ht/~rjarry/aerc/worker/types"
 )
 
@@ -51,6 +52,9 @@ func (imapw *IMAPWorker) handleMoveMessages(msg *types.MoveMessages) {
 	drain := imapw.drainUpdates()
 	defer drain.Close()
 
+	// Build provider-dependent EXPUNGE handler.
+	imapw.BuildExpungeHandler(models.UidToUint32List(msg.Uids))
+
 	uids := toSeqSet(msg.Uids)
 	if err := imapw.client.UidMove(uids, msg.Destination); err != nil {
 		imapw.worker.PostMessage(&types.Error{

worker/imap/seqmap.go

@@ -1,8 +1,11 @@
 package imap
 
 import (
+	"slices"
 	"sort"
 	"sync"
+
+	"git.sr.ht/~rjarry/aerc/lib/log"
 )
 
 type SeqMap struct {
@@ -54,6 +57,41 @@ func (s *SeqMap) Put(uid uint32) {
 	s.lock.Unlock()
 }
 
+// Take a snapshot of the SequenceNumber=>UID mappings for the given UIDs,
+// remove those UIDs from the SeqMap, and return the snapshot it to the caller,
+// as well as the loweest sequence number it contains.
+func (s *SeqMap) Snapshot(uids []uint32) (map[uint32]uint32, uint32) {
+	// Take the snapshot.
+	snapshot := make(map[uint32]uint32)
+	var minSequenceNum uint32 = 0
+	var snapshotSeqNums []uint32
+	s.lock.Lock()
+	for num, uid := range s.m {
+		if slices.Contains(uids, uid) {
+			// IMAP sequence numbers start at 1
+			seqNum := uint32(num) + 1
+			snapshotSeqNums = append(snapshotSeqNums, seqNum)
+			if minSequenceNum == 0 {
+				minSequenceNum = seqNum
+			}
+			snapshot[seqNum] = uid
+		}
+	}
+	s.lock.Unlock()
+
+	// Remove the snapshotted mappings from the sequence; we need to do it from
+	// the highest to the lowest key since a SeqMap.Pop moves all the items on
+	// the right of the popped sequence number by one position to the left.
+	for i := len(snapshotSeqNums) - 1; i >= 0; i-- {
+		_, ok := s.Pop(snapshotSeqNums[i])
+		if !ok {
+			log.Errorf("Unable to pop %d from SeqMap", snapshotSeqNums[i])
+		}
+	}
+
+	return snapshot, minSequenceNum
+}
+
 // Pop removes seqnum from the SeqMap. seqnum must be a valid seqnum, ie
 // [1:size of mailbox]
 func (s *SeqMap) Pop(seqnum uint32) (uint32, bool) {

worker/imap/seqmap_test.go

@@ -82,4 +82,35 @@ func TestSeqMap(t *testing.T) {
 	wg.Wait()
 
 	assert.Equal(0, seqmap.Size())
+
+	//
+	// Test snapshotting
+	//
+
+	// The sequence is [ 21, 24, 1107, 1982, 2399, 27892, 32000 ] (1 is at
+	// position 1, 24 position 2, etc)
+	seqmap.Initialize([]uint32{21, 42, 1107, 1982, 2390, 27892, 32000})
+
+	// Snapshot [ 21, 1107, 27892, 1234567 ]; they're all present in the
+	// sequence except 27982
+	snap, min := seqmap.Snapshot([]uint32{21, 1107, 27892, 1234567})
+
+	// Verify that we did snapshot [ 1 => 21, 3=>1107, 6=>27892 ]
+	assert.Equal(3, len(snap))
+	assert.Equal(uint32(1), min)
+	assert.Equal(snap[1], uint32(21))
+	assert.Equal(snap[3], uint32(1107))
+	assert.Equal(snap[6], uint32(27892))
+
+	// Vefify that the snapshotted items have been removed from the sequence,
+	// that therefore became [ 42, 1982, 2390, 32000 ]
+	assert.Equal(4, seqmap.Size())
+	o1, _ := seqmap.Get(1)
+	assert.Equal(uint32(42), o1)
+	o2, _ := seqmap.Get(2)
+	assert.Equal(uint32(1982), o2)
+	o3, _ := seqmap.Get(3)
+	assert.Equal(uint32(2390), o3)
+	o4, _ := seqmap.Get(4)
+	assert.Equal(uint32(32000), o4)
 }

worker/imap/worker.go

@@ -60,6 +60,7 @@ type imapConfig struct {
 	cacheEnabled       bool
 	cacheMaxAge        time.Duration
 	useXGMEXT          bool
+	expungePolicy      int
 }
 
 type IMAPWorker struct {
@@ -70,6 +71,7 @@ type IMAPWorker struct {
 	updates   chan client.Update
 	worker    types.WorkerInteractor
 	seqMap    SeqMap
+	expunger  *ExpungeHandler
 	delimiter string
 
 	idler    *idler
@@ -295,6 +297,13 @@ func (w *IMAPWorker) handleImapUpdate(update client.Update) {
 				msg.Uid = uid
 			}
 		}
+		if w.expunger != nil && w.expunger.IsExpunging(msg.Uid) {
+			// After we marked messages as Deleted and before expunging them
+			// (i.e. the worker's ExpungeHandler is not nil), some IMAP servers
+			// will send a MessageUpdate confirming that the messages have been
+			// marked as Deleted. We should simply ignore those.
+			return
+		}
 		if int(msg.SeqNum) > w.seqMap.Size() {
 			w.seqMap.Put(msg.Uid)
 		}
@@ -309,7 +318,12 @@ func (w *IMAPWorker) handleImapUpdate(update client.Update) {
 			Unsolicited: true,
 		}, nil)
 	case *client.ExpungeUpdate:
-		if uid, found := w.seqMap.Pop(update.SeqNum); !found {
+		if w.expunger == nil {
+			// This can happen for a really unsolicited deletion (e.g. we never
+			// did a delete nor a move since aerc started and something was
+			// deleted by another client).
+			w.worker.Errorf("ExpungeUpdate unknown seqnum: %d", update.SeqNum)
+		} else if uid, found := w.expunger.PopSequenceNumber(update.SeqNum); !found {
 			w.worker.Errorf("ExpungeUpdate unknown seqnum: %d", update.SeqNum)
 		} else {
 			w.worker.PostMessage(&types.MessagesDeleted{
@@ -411,3 +425,7 @@ func (w *IMAPWorker) PathSeparator() string {
 	}
 	return w.delimiter
 }
+
+func (w *IMAPWorker) BuildExpungeHandler(uids []uint32) {
+	w.expunger = NewExpungeHandler(w, uids)
+}